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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
