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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert 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:
> 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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98746