JonChesterfield added a comment.
In D94961#2533848 <https://reviews.llvm.org/D94961#2533848>, @jdoerfert wrote:
> Overall, looks reasonable. I'm not a fan of the way things are hooked up but
> that is also true for the NVPTX toolchain and for now unavoidable I guess.
This is loosely based on the nvptx and hip toolchain files, but looking at
others in clang the style seems similar.
================
Comment at: clang/lib/Driver/Driver.cpp:2999
+ if (VStr.startswith("-march=")) {
+ if (StringRef(VStr.str().substr(7)).startswith("gfx"))
+ IsAMDGCN = true;
----------------
jdoerfert wrote:
> Why the `substr` copy?
As in `VStr.startswith("-march=gfx")`? Slightly prettier. Not sure ad hoc arg
parsing like this is ever good though
================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.h:72
+ bool isPICDefaultForced() const override { return false; }
+ bool SupportsProfiling() const override { return false; }
+
----------------
jdoerfert wrote:
> What is the coding style here? Pick upper case or lower case, form the above
> I assume upper case.
Yeah, should be `supportsProfiling()` as it's a function. Should probably
propose that as a minimal patch to the existing code. There may be other casing
mistakes. ConstructJob looks suspect, but all the files call it that instead of
constructJob, so maybe I'm missing a part of the style guide.
================
Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:65
+ .Case("g", "1")
+ .Default("2");
+ }
----------------
jdoerfert wrote:
> To verify, O0 is not mapped to O2, correct?
I think `opt -O0` is an error, though it does look like this will rewrite it to
O2 instead. Which seems bad.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94961/new/
https://reviews.llvm.org/D94961
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
