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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
