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

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added a comment.

Looks much simpler, thanks!



================
Comment at: clang/lib/Driver/Driver.cpp:755
+                      *this, TT, *HostTC, C.getInputArgs(), Action::OFK_OpenMP);
+                else if (TT.isAMDGCN())
+                  DeviceTC =
----------------
Minor suggestion,
```
if (TT.isNVPTX() {
 ...
} else {
  assert(TT.isAMDGCN());
  ...
}```

? Semantically equivalent, but `if () else if ()` looks like there is a missing else clause.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:43
+
+static void addOptLevelArgs(const llvm::opt::ArgList &Args,
+                            llvm::opt::ArgStringList &CmdArgs) {
----------------
Maybe rename this since opt is no longer involved. addLLCOptArg or similar? There's probably the same logic in another toolchain


================
Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:64
+                 .Case("g", "1")
+                 .Default("2");
+    }
----------------
Default("0") and update the comment 'we map anything else'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94961