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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/Basic/Cuda.h:61
+  GFX900,
+  GFX902,
   LAST,
----------------
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.


================
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]>,
----------------
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.


================
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;
----------------
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.


https://reviews.llvm.org/D45212