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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

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. Some nits below.



================
Comment at: clang/lib/Driver/Driver.cpp:2999
+            if (VStr.startswith("-march=")) {
+              if (StringRef(VStr.str().substr(7)).startswith("gfx"))
+                IsAMDGCN = true;
----------------
Why the `substr` copy?


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.h:72
+  bool isPICDefaultForced() const override { return false; }
+  bool SupportsProfiling() const override { return false; }
+
----------------
What is the coding style here? Pick upper case or lower case, form the above I assume upper case.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:65
+                 .Case("g", "1")
+                 .Default("2");
+    }
----------------
To verify, O0 is not mapped to O2, correct?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94961/new/

https://reviews.llvm.org/D94961