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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Generally OK, the more I look at it the more things pop up. Other than my comments, I'm fine with this going in.



================
Comment at: clang/lib/Driver/Driver.cpp:767
+              }
+              TC = AMDGPUOpenMPTC.get();
             } else
----------------
 what if we say `TT.isNVPTX() || TT.isAMDGCN()` in line 745, rename the Cuda things to "device" and the toolchain stuff is in a conditional? I see the triple is hardcoded here for some reason and I don't know why not to use TT. Neverhteless, it seems less copying is better, we will eventually have more architectures and 3 * 13 lines means 3 times the bugs and 3 times the required changes in the future.


================
Comment at: clang/lib/Driver/Driver.cpp:3004
+        }
+      }
+      // amdgcn does not support linking of object files, therefore we skip
----------------
This does not look right. IsAMDGCN is not a question that should be answered by a `-Xopenmp-target` flag. 

In general, why skip these passes here. Isn't what you really want to do, pass `-emit-llvm[-bc]` to the device compilation job?


================
Comment at: clang/lib/Driver/ToolChains/AMDGPUOpenMP.cpp:66
+                 .Default("2");
+    }
+    CmdArgs.push_back(Args.MakeArgString("-O" + OOpt));
----------------
Just to make it clear, we should default to O0.


================
Comment at: clang/test/Driver/amdgpu-openmp-toolchain.c:2
+// REQUIRES: amdgpu-registered-target
+// RUN:   %clang -### --target=x86_64-unknown-linux-gnu -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa -Xopenmp-target=amdgcn-amd-amdhsa -march=gfx906 %s 2>&1 \
+// RUN:   | FileCheck %s
----------------
Would this at all work without the march flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94961