yaxunl marked 4 inline comments as done. yaxunl added inline comments.
================ Comment at: include/clang/Basic/Cuda.h:61 + GFX900, + GFX902, LAST, ---------------- Hahnfeld wrote: > rjmccall wrote: > > yaxunl wrote: > > > rjmccall wrote: > > > > Does this actually have anything to do with HIP? You have a lot of > > > > changes in this patch which seem to just be about supporting more GPU > > > > revisions. > > > This patch not only adds support of HIP language mode but also adds > > > support of amdgpu to CUDA toolchain. > > > > > > Currently HIP extension is only supported by amdgpu although in the > > > future it may be supported by other targets. > > I understand that, but I think you can separate those two patches without > > too much difficulty. > There already is D42800 with lots of unanswered comments, so those need to be > addressed first. Will split the changes for supporting amdgcn to another review. ================ Comment at: include/clang/Basic/Cuda.h:61 + GFX900, + GFX902, LAST, ---------------- yaxunl wrote: > Hahnfeld wrote: > > rjmccall wrote: > > > yaxunl wrote: > > > > rjmccall wrote: > > > > > Does this actually have anything to do with HIP? You have a lot of > > > > > changes in this patch which seem to just be about supporting more GPU > > > > > revisions. > > > > This patch not only adds support of HIP language mode but also adds > > > > support of amdgpu to CUDA toolchain. > > > > > > > > Currently HIP extension is only supported by amdgpu although in the > > > > future it may be supported by other targets. > > > I understand that, but I think you can separate those two patches without > > > too much difficulty. > > There already is D42800 with lots of unanswered comments, so those need to > > be addressed first. > Will split the changes for supporting amdgcn to another review. Will address those comments in the split review. ================ Comment at: include/clang/Driver/Options.td:556 +def offload_archs : Joined<["--"], "offload-archs=">, Flags<[DriverOption]>, + HelpText<"List of offload architectures for CUDA/HIP/OpenMP (e.g. sm_35,gfx803).">; def no_cuda_gpu_arch_EQ : Joined<["--"], "no-cuda-gpu-arch=">, Flags<[DriverOption]>, ---------------- rjmccall wrote: > yaxunl wrote: > > rjmccall wrote: > > > Do we absolutely need the non-CUDA-related aliases here? We generally > > > try to be good about namespacing extension-specific language options. > > > > > > I understand that you're probably trying to maintain command-line > > > compatibility with some existing toolchain, but if it's possible to avoid > > > this, I would be much happier. > > There were discussions about a uniform clang option for offloading sub-arcs > > > > http://lists.llvm.org/pipermail/llvm-dev/2017-February/109896.html > > > > the consensus seem to be -offload-arch or -offload-archs. > > > > This patch attempts to make that transition to use this new option. > > > > We can separate this change to a different patch though. > I don't mind the `-offload-*` options; I'm more concerned about `--host-only` > and so on. Will drop those options for now. Since HIP is just a language mode of CUDA, it is OK to use CUDA options e.g. 'cuda-device-only' for HIP. ================ Comment at: include/clang/Driver/ToolChain.h:124 mutable std::unique_ptr<Tool> Clang; + mutable std::unique_ptr<Tool> Backend; mutable std::unique_ptr<Tool> Assemble; ---------------- rjmccall wrote: > yaxunl wrote: > > rjmccall wrote: > > > "Backend" is a really generic name for this thing that is probably > > > hyper-specific to the CUDA translation model. > > Agree. This tool actually links bunch of bitcode libraries (so called > > device libraries). For non-GPU targets, this is usually unnecessary since > > they support ISA-level linking. However most GPU targets do not support > > that, therefore they need this stage. > > > > How about renaming it as BitcodeLink? > DeviceLibraryLink, maybe? I wouldn't want someone to think this was related > to ordinary LTO. That sounds good. Will change to that. https://reviews.llvm.org/D45212 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
