[PATCH] D133726: [OpenMP][AMDGPU] Link bitcode ROCm device libraries per-TU

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/AMDGPU.cpp:717
 AMDGPUToolChain::getGPUArch(const llvm::opt::ArgList &DriverArgs) const {
-  return getProcessorFromTargetID(
-      getTriple(), DriverArgs.getLastArgValue(options::OPT_mcpu_EQ));
+  if (DriverArgs.hasArg(options::OPT_mcpu_EQ))
+    return getProcessorFromTargetID(
----------------
jhuber6 wrote:
> yaxunl wrote:
> > jhuber6 wrote:
> > > yaxunl wrote:
> > > > can we emit an error if both -march and -mcpu are specified and the values are different? This is a potential source of error.
> > > I'm not exactly sure about the semantics for this, maybe someone else can help. AFAIK `-mcpu` is sometimes treated as an alias of `-mtune` which is generally implied by `-march`. But having `-march` and `-mtune` state different things isn't technically disallowed. `-march` seems to imply that we generate code that can run on a certain architecture. Since these are in some respects equivalent it would probably be fine to just take the last one specified.
> > Some toolchains use -march to specify the target processor, some toolchains use -mcpu to specify the target processor. For AMDGPU toolchain, it uses -mcpu to specify the target processor in the beginning for OpenCL. Then we add HIP support and let HIPAMD toolchain adopt using -mcpu to specify the target processor. It is better to have a consistent way for specifying target processor for AMDGPU toolchains.
> > 
> > HIPAMD toolchain does this by https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/HIPAMD.cpp#L275 .
> > 
> > Probably AMDGPUOpenMP toolchain can make a similar change? Then we have a consistent way to specify target processor for AMDGPU toolchains.
> The rest of the OpenMP toolchain uses `-march` so I don't see a compelling reason to change it. If this is a stopper I'll just remove the changes to this function and check `-march` directly in the new `getROCmDeviceLibs` function for the OpenMP toolchain.
There are a bunch of places in AMDGPU and HIPAMD toolchains that check the `-mcpu` option. If we allow -march as an alternative way to specify the target processor for the AMDGPU toolchain, it will introduce inconsistency and probably incur more changes.

Probably leaving this code in AMDGPUOpenMP toolchain is a better choice for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133726