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.
Differential D45212
Add HIP toolchain yaxunl on Apr 3 2018, 9:27 AM. Authored by
Details 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 TimelineComment Actions The other changes I see here seem reasonable, but please do split the patch.
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...
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 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. 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.
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
Comment Actions One small nit. LGTM otherwise.
|
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.