[PATCH] D45212: [HIP] Let CUDA toolchain support HIP language mode and amdgpu

[Yaxun Liu via Phabricator via cfe-commits]

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