This patch adds HIP toolchain to support HIP language mode. It includes:
Create specific compiler jobs for HIP.
Choose specific libraries for HIP.
With contribution from Greg Rodgers.
Paths
| Differential D45212
Add HIP toolchain ClosedPublic Authored by yaxunl on Apr 3 2018, 9:27 AM.
Details Summary This patch adds HIP toolchain to support HIP language mode. It includes: Create specific compiler jobs for HIP. Choose specific libraries for HIP. With contribution from Greg Rodgers.
Diff Detail Event Timelineyaxunl retitled this revision from [WIP][HIP] Let CUDA toolchain support HIP language mode to [HIP] Let CUDA toolchain support HIP language mode. Comment ActionsFixed typo which causes lit test failures. Now all lit tests pass. Comment Actions The other changes I see here seem reasonable, but please do split the patch.
yaxunl retitled this revision from [HIP] Let CUDA toolchain support HIP language mode to [HIP] Let CUDA toolchain support HIP language mode and amdgpu.Apr 4 2018, 7:29 AM yaxunl added inline comments.
yaxunl added inline comments.
yaxunl marked an inline comment as done. Comment ActionsRevised by reviewers' comments, including comments from previous review. Comment Actions This looks okay to me, but I think it would better if someone with more expertise in the design of the driver and frontend code could review this.
Comment Actions Can this revision be split further? The summary mentions many things that might make up multiple independent changes...
yaxunl added inline comments. Comment Actions
I separated file type changes to https://reviews.llvm.org/D45489 and deferred some other changes for future. It is kind of difficult to split this patch further since they depend on each other. Comment Actions
You can add Related Revisions to make this easy to see. Comment Actions I'd still prefer if someone with more driver-design expertise weighed in, but we might not have any specialists there. LGTM, although you might consider changing your tests a bit: FileCheck recently added support for a -D argument where you can predefine variables in the command line. But that's just a suggestion, not something I'm asking you to do as part of review. This revision is now accepted and ready to land.Apr 13 2018, 12:16 AM Comment Actions
I think you should at least give @tra the possibility to take a look. Last time we essentially ended up reverting a patch. Comment Actions Sorry about the delay. I've been out for few days.
This revision now requires changes to proceed.Apr 17 2018, 4:30 PM yaxunl edited parent revisions, added: D45441: [HIP] Add predefined macros __HIPCC__ and __HIP_DEVICE_COMPILE__; removed: D44984: [HIP] Add hip input kind and codegen for kernel launching. yaxunl added inline comments.
yaxunl marked an inline comment as done. Comment ActionsRemove manually added passes from opt and add -mtriple. yaxunl removed a parent revision: D45441: [HIP] Add predefined macros __HIPCC__ and __HIP_DEVICE_COMPILE__.May 4 2018, 8:42 PM yaxunl added a child revision: D46489: [HIP] Let assembler output bitcode for amdgcn.May 4 2018, 9:06 PM yaxunl removed a child revision: D46489: [HIP] Let assembler output bitcode for amdgcn.May 16 2018, 12:40 PM Comment Actions Hi, Sorry about the long silence. I'm back to continue the reviews. I'll handle what I can today and will continue with the rest on Tuesday. It looks like patch description needs to be updated:
I don't see anything related to offload-bundler in this patch any more.
Comment Actions One more thing -- it would be really good to add some tests to make sure your commands are constructed the way you want. Comment Actions
You are right. Using clang-offload-bundler to create binary for device ISA has been moved to another patch. Will update the description of this patch.
will do
yaxunl marked 19 inline comments as done. yaxunl retitled this revision from [HIP] Let CUDA toolchain support HIP language mode and amdgpu to Add HIP toolchain. Comment ActionsRevised by Artem's comments.
yaxunl added inline comments.
Comment Actions One small nit. LGTM otherwise.
This revision is now accepted and ready to land.May 23 2018, 1:42 PM Closed by commit rC333484: Add HIP toolchain (authored by yaxunl). · Explain WhyMay 29 2018, 5:57 PM This revision was automatically updated to reflect the committed changes. yaxunl marked an inline comment as done.
Revision Contents
Diff 140811 include/clang/Basic/Cuda.h
include/clang/Basic/DiagnosticDriverKinds.td
include/clang/Driver/Options.td
include/clang/Driver/ToolChain.h
include/clang/Driver/Types.def
lib/Basic/Cuda.cpp
lib/Basic/Targets.h
lib/Basic/Targets.cpp
lib/Basic/Targets/AMDGPU.h
lib/Basic/Targets/AMDGPU.cpp
lib/Basic/Targets/NVPTX.cpp
lib/Driver/Driver.cpp
lib/Driver/SanitizerArgs.cpp
lib/Driver/ToolChain.cpp
lib/Driver/ToolChains/Clang.cpp
lib/Driver/ToolChains/Cuda.h
lib/Driver/ToolChains/Cuda.cpp
lib/Driver/ToolChains/Gnu.cpp
lib/Driver/Types.cpp
lib/Frontend/CompilerInstance.cpp
test/Driver/cuda-phases.cu
|
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.