rjmccall added inline comments.

Comment at: include/clang/Basic/Cuda.h:61
+  GFX900,
+  GFX902,
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. 
 def no_cuda_gpu_arch_EQ : Joined<["--"], "no-cuda-gpu-arch=">, 
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.


cfe-commits mailing list

Reply via email to