[PATCH] D94961: [OpenMP] Add OpenMP offloading toolchain for AMDGPU

[Jon Chesterfield via Phabricator via cfe-commits]

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