[PATCH] D98746: [clang][amdgpu] Use implicit code object default

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1115-1124
+  // Currently defaults to 3 in AMDGPUBaseInfo.cpp
+  // Using that default lets clang emit IR for amdgcn when llvm has been built
+  // without that target, provided the user wants this code object version
+  if (CodeObjVer != 3) {
+    CmdArgs.insert(CmdArgs.begin() + 1,
+                   Args.MakeArgString(Twine("--amdhsa-code-object-version=") +
+                                      Twine(CodeObjVer)));
----------------
JonChesterfield wrote:
> yaxunl wrote:
> > jdoerfert wrote:
> > > JonChesterfield wrote:
> > > > t-tye wrote:
> > > > > This seem rather fragile. If the backend default changes and this code is not updated it will likely result in creating the wrong code object version.
> > > > That is true, though numbers 2 through 4 are used in getOrCheckAMDGPUCodeObjectVersion with 3 as the default there too.
> > > > 
> > > > Equally, today if we change the default in clang and forget to update the backend, we won't notice because we always pass this argument.
> > > > 
> > > > What we have at present is the back end has the state and the front end chooses the default. Needing to write '3' in both places is a symptom of that. Perhaps the back end should not have a default for it, requiring this to always be passed, at which point we are going to have a bad time building amdgpu openmp by default.
> > > > 
> > > > This change decouples the two for the (likely common) case where the user has not chosen to override the value. It's not ideal, but equally we don't change the version that often and there are lit tests that notice if we get it wrong. Plus stuff seems to break if the code object version is not as expected.
> > > > 
> > > > There may be a better solution. Target specific variable set by clang feels like something that will occur elsewhere.
> > > Arguably, the backend should have a default, the option should be used by users that don't like the default.
> > > 
> > I think a reasonable behavior of clang is to add -mllvm --amdhsa-code-object-version=x only if users use -mcode-object-version=x or equivalent explicitly. Otherwise clang should not add -mllvm --amdhsa-code-object-version=x and let the backend decides the default code object version.
> > 
> > @t-tye @kzhuravl what do you think? thanks.
> That sounds right to me. It's slightly more complicated to implement than this, will need to rework getOrCheckAMDGPUCodeObjectVersion to distinguish between the get and check parts.
well as Tony pointed out, clang itself needs to know the default code object version, since it need it to emit the correct bundle entry ID.

I think the issue is that clang is used to compile HIP to bitcode for amdgpu when amdgpu is not a registered target. Although there is a slight concern that whether this will always result in valid bitcode for amdgpu, this may be OK since the backend should be able to consume bitcode generated with -O0.

If we are confident the LLVM option --amdhsa-code-object-version=x is only needed for emitting ISA and can be omitted for emitting bitcode, a fix may be to check output type and skip emitting this LLVM option if the output is bitcode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98746