[Clang][AMDGPU] Fix handling of -mcode-object-version=none arg

[saiislam]

Summary

-mcode-object-version=none is a special argument which allows
abi-agnostic code to be generated for device runtime libraries.

[arsenm]

missing tests

clang/lib/Driver/ToolChains/Clang.cpp

1066

don't need to go through std::string? stick with Twine everywhere?

clang/lib/Driver/ToolChains/CommonArgs.cpp

2309

missing space after if, also return on separate line. Also why starts with, and not ==?

[jhuber6]

clang/include/clang/Basic/TargetOptions.h

90

Typically we just put a COV_LAST to indicate that it's over the accepted enumerations.

clang/lib/Driver/ToolChain.cpp

1364

Is this flag not in the m group? It should be caught here right?

clang/lib/Driver/ToolChains/Clang.cpp

1058

Use clang-format.

1066

You shouldn't assign to a Twine, but in general I think we should probably put this ternary in-line with the other stuff to avoid the temporary.

The handling here is a little confusing, we do

Args.getLastArg(options::OPT_mcode_object_version_EQ);

Which expects a number, if it's not present we get an empty string which default converts to zero which we then convert into "none"?

[yaxunl]

-mcode-object-version=none was intentionally designed to work with clang -cc1 only, since it does not work with clang driver if users link with device library. Device library can still use it by using it with -Xclang.

[jhuber6]

In D156928#4555121, @yaxunl wrote:

-mcode-object-version=none was intentionally designed to work with clang -cc1 only, since it does not work with clang driver if users link with device library. Device library can still use it by using it with -Xclang.

If the intended use is the deviceRTL then that should be sufficient.

[saiislam]

In D156928#4555121, @yaxunl wrote:

-mcode-object-version=none was intentionally designed to work with clang -cc1 only, since it does not work with clang driver if users link with device library. Device library can still use it by using it with -Xclang.

Thanks for the tip @yaxunl . I will abandon this revision and use Xclang for passing cov_none to devicertl.

[JonChesterfield]

What does code objects version= none mean?

[arsenm]

In D156928#4561811, @JonChesterfield wrote:

What does code objects version= none mean?

Handle any version

[JonChesterfield]

In D156928#4561849, @arsenm wrote:

In D156928#4561811, @JonChesterfield wrote:

What does code objects version= none mean?

Handle any version

So... That should be the default, right? Emit IR that the back end specialises. Or, ideally, the only behaviour as far as the front end is concerned.

[yaxunl]

In D156928#4561890, @JonChesterfield wrote:

In D156928#4561849, @arsenm wrote:

In D156928#4561811, @JonChesterfield wrote:

What does code objects version= none mean?

Handle any version

So... That should be the default, right? Emit IR that the back end specialises. Or, ideally, the only behaviour as far as the front end is concerned.

Code in the device library depends on a control variable about the code object version. Specifying the code object version in Clang allows internalizing that variable and optimizing code depending on it as early as possible. Not specifying it with Clang will require an LLVM pass in amdgpu backend to define that control variable after linking and it has to have an external linkage. This may lose optimization. Also, you need a way to not specify it in FE but specify it in BE. This just complicates things without much benefits.

[yaxunl]

In D156928#4562023, @yaxunl wrote:

In D156928#4561890, @JonChesterfield wrote:

In D156928#4561849, @arsenm wrote:

In D156928#4561811, @JonChesterfield wrote:

What does code objects version= none mean?

Handle any version

So... That should be the default, right? Emit IR that the back end specialises. Or, ideally, the only behaviour as far as the front end is concerned.

Code in the device library depends on a control variable about the code object version. Specifying the code object version in Clang allows internalizing that variable and optimizing code depending on it as early as possible. Not specifying it with Clang will require an LLVM pass in amdgpu backend to define that control variable after linking and it has to have an external linkage. This may lose optimization. Also, you need a way to not specify it in FE but specify it in BE. This just complicates things without much benefits.

On second thoughts, this may inspire us about eliminating not just the code object control variable but all device library control variables.

Basically in Clang we can emit a module flag about required control variables and do not link the device libs that implement these control variables.

Then we add an LLVM pass at the very beginning of the optimization pipeline which checks that module flag and defines those control variables with internal linkage. This way, we should be able to get rid of those control variable device libs without losing performance.

[JonChesterfield]

Or, the front end could define those objects directly, without importing IR files that define the objects with the content clang used to choose the object file. E.g. instead of the argument daz=off (spelled differently) finding a file called daz.off.ll that defines variable called daz with a value 0, that argument could define that variable. I think @jhuber6 has a partial patch trying to do that.

If we were more ambitious, we could use intrinsics that are folded reliably at O0 instead of magic variables that hopefully get constant folded. That would kill a bunch of O0 bugs.

In general though, splicing magic variables in the front end seems unlikely to be performance critical relative to splicing them in at the start of the backend.

[yaxunl]

In D156928#4562239, @JonChesterfield wrote:

Or, the front end could define those objects directly, without importing IR files that define the objects with the content clang used to choose the object file. E.g. instead of the argument daz=off (spelled differently) finding a file called daz.off.ll that defines variable called daz with a value 0, that argument could define that variable. I think @jhuber6 has a partial patch trying to do that.

If we were more ambitious, we could use intrinsics that are folded reliably at O0 instead of magic variables that hopefully get constant folded. That would kill a bunch of O0 bugs.

In general though, splicing magic variables in the front end seems unlikely to be performance critical relative to splicing them in at the start of the backend.

Some control variables are per-module. Clang cannot emit control variables that have different values for different modules. Intrinsics should work since it can take an argument as its value.

[JonChesterfield]

Tangent below. Main thrust is this =none feature should be called "default", not none, and be the default, and there should be no feature called ABI=none.

In D156928#4562412, @yaxunl wrote:

Some control variables are per-module. Clang cannot emit control variables that have different values for different modules. Intrinsics should work since it can take an argument as its value.

Sure it can. In the most limited case, we could exactly replace the variables imported from source IR in the device libs based on command line variables with the same globals with the same value and attributes. That would be NFC except it would no longer matter if the devicertl source was present.

There's just also other better things to be done as well, like not burning ABI decisions in the front end, or not using magic variables at all. But it's definitely not necessary to write constants in IR files that encode the same information as the command line flag. That's a whole indirect no-op that doesn't need to be there.

[jhuber6]

In D156928#4562239, @JonChesterfield wrote:

Or, the front end could define those objects directly, without importing IR files that define the objects with the content clang used to choose the object file. E.g. instead of the argument daz=off (spelled differently) finding a file called daz.off.ll that defines variable called daz with a value 0, that argument could define that variable. I think @jhuber6 has a partial patch trying to do that.

If we were more ambitious, we could use intrinsics that are folded reliably at O0 instead of magic variables that hopefully get constant folded. That would kill a bunch of O0 bugs.

In general though, splicing magic variables in the front end seems unlikely to be performance critical relative to splicing them in at the start of the backend.

I think @saiislam is working on a patch that will handle that. We'll have clang emit some global that OpenMP uses.

[saiislam]

In D156928#4565506, @jhuber6 wrote:

In D156928#4562239, @JonChesterfield wrote:

Or, the front end could define those objects directly, without importing IR files that define the objects with the content clang used to choose the object file. E.g. instead of the argument daz=off (spelled differently) finding a file called daz.off.ll that defines variable called daz with a value 0, that argument could define that variable. I think @jhuber6 has a partial patch trying to do that.

If we were more ambitious, we could use intrinsics that are folded reliably at O0 instead of magic variables that hopefully get constant folded. That would kill a bunch of O0 bugs.

In general though, splicing magic variables in the front end seems unlikely to be performance critical relative to splicing them in at the start of the backend.

I think @saiislam is working on a patch that will handle that. We'll have clang emit some global that OpenMP uses.

Thanks Joseph.
Yes, I have abandoned this patch and using -Xclang -mcode-object-version=none option in the patch to enable cov5 support for OpenMP.

clang/include/clang/Basic/TargetOptions.h

public:
   /// \brief Enumeration value for AMDGPU code object version, which is the
   /// code object version times 100.
   enum CodeObjectVersionKind {
     COV_None,
     COV_2 = 200,
     COV_3 = 300,
     COV_4 = 400,
     COV_5 = 500,
+    COV_Default = 400,
+    COV_MAX = 500
   };
   /// \brief Code object version for AMDGPU.
   CodeObjectVersionKind CodeObjectVersion = CodeObjectVersionKind::COV_None;
 
   /// \brief Enumeration values for AMDGPU printf lowering scheme
   enum class AMDGPUPrintfKind {
     /// printf lowering scheme involving hostcalls, currently used by HIP
     /// programs by default

clang/lib/Driver/ToolChain.cpp

llvm::opt::DerivedArgList *ToolChain::TranslateOpenMPTargetArgs(
     const llvm::opt::DerivedArgList &Args, bool SameTripleAsHost,
     SmallVectorImpl<llvm::opt::Arg *> &AllocatedArgs) const {
   DerivedArgList *DAL = new DerivedArgList(Args.getBaseArgs());
   const OptTable &Opts = getDriver().getOpts();
   bool Modified = false;
 
   // Handle -Xopenmp-target flags
   for (auto *A : Args) {
+    if (A->getOption().matches(options::OPT_mcode_object_version_EQ))
+      DAL->append(A);
+
     // Exclude flags which may only apply to the host toolchain.
     // Do not exclude flags when the host triple (AuxTriple)
     // matches the current toolchain triple. If it is not present
     // at all, target and host share a toolchain.
     if (A->getOption().matches(options::OPT_m_Group)) {
       if (SameTripleAsHost)
         DAL->append(A);
       else
         Modified = true;
       continue;
     }
 
     unsigned Index;

clang/lib/Driver/ToolChains/Clang.cpp

static void handleAMDGPUCodeObjectVersionOptions(const Driver &D,
                                                  bool IsCC1As = false) {
   // If no version was requested by the user, use the default value from the
   // back end. This is consistent with the value returned from
   // getAMDGPUCodeObjectVersion. This lets clang emit IR for amdgpu without
   // requiring the corresponding llvm to have the AMDGPU target enabled,
   // provided the user (e.g. front end tests) can use the default.
   if (haveAMDGPUCodeObjectVersionArgument(D, Args)) {
     unsigned CodeObjVer = getAMDGPUCodeObjectVersion(D, Args);
+    if(CodeObjVer != 0) {
     CmdArgs.insert(CmdArgs.begin() + 1,
                    Args.MakeArgString(Twine("--amdhsa-code-object-version=") +
                                       Twine(CodeObjVer)));
     CmdArgs.insert(CmdArgs.begin() + 1, "-mllvm");
+    }
     // -cc1as does not accept -mcode-object-version option.
-    if (!IsCC1As)
+    if (!IsCC1As) {
+      std::string CodeObjVerStr = (CodeObjVer ? Twine(CodeObjVer) : "none").str();
       CmdArgs.insert(CmdArgs.begin() + 1,
                      Args.MakeArgString(Twine("-mcode-object-version=") +
-                                        Twine(CodeObjVer)));
+                                        CodeObjVerStr));
+    }
   }
 }
 
 void Clang::AddPreprocessingOptions(Compilation &C, const JobAction &JA,
                                     const Driver &D, const ArgList &Args,
                                     ArgStringList &CmdArgs,
                                     const InputInfo &Output,
                                     const InputInfoList &Inputs) const {

clang/lib/Driver/ToolChains/CommonArgs.cpp

 #include "Arch/VE.h"
 #include "Arch/X86.h"
 #include "HIPAMD.h"
 #include "Hexagon.h"
 #include "MSP430.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/ObjCRuntime.h"
+#include "clang/Basic/TargetOptions.h"
 #include "clang/Basic/Version.h"
 #include "clang/Config/config.h"
 #include "clang/Driver/Action.h"
 #include "clang/Driver/Compilation.h"
 #include "clang/Driver/Driver.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/InputInfo.h"
 #include "clang/Driver/Job.h"

 
 static llvm::opt::Arg *
 getAMDGPUCodeObjectArgument(const Driver &D, const llvm::opt::ArgList &Args) {
   return Args.getLastArg(options::OPT_mcode_object_version_EQ);
 }
 
 void tools::checkAMDGPUCodeObjectVersion(const Driver &D,
                                          const llvm::opt::ArgList &Args) {
-  const unsigned MinCodeObjVer = 2;
-  const unsigned MaxCodeObjVer = 5;
 
   if (auto *CodeObjArg = getAMDGPUCodeObjectArgument(D, Args)) {
     if (CodeObjArg->getOption().getID() ==
         options::OPT_mcode_object_version_EQ) {
-      unsigned CodeObjVer = MaxCodeObjVer;
-      auto Remnant =
-          StringRef(CodeObjArg->getValue()).getAsInteger(0, CodeObjVer);
-      if (Remnant || CodeObjVer < MinCodeObjVer || CodeObjVer > MaxCodeObjVer)
+      unsigned CodeObjVer = TargetOptions::COV_Default / 100;
+      auto CovStr = StringRef(CodeObjArg->getValue());
+      if(CovStr.starts_with("none")) return;
+  
+      CovStr.getAsInteger(0, CodeObjVer);
+      if (CodeObjVer < TargetOptions::COV_None || CodeObjVer > TargetOptions::COV_MAX)
         D.Diag(diag::err_drv_invalid_int_value)
             << CodeObjArg->getAsString(Args) << CodeObjArg->getValue();
     }
   }
 }
 
 unsigned tools::getAMDGPUCodeObjectVersion(const Driver &D,
                                            const llvm::opt::ArgList &Args) {
-  unsigned CodeObjVer = 4; // default
-  if (auto *CodeObjArg = getAMDGPUCodeObjectArgument(D, Args))
-    StringRef(CodeObjArg->getValue()).getAsInteger(0, CodeObjVer);
+
+  unsigned CodeObjVer = TargetOptions::COV_Default / 100; // default
+  if (haveAMDGPUCodeObjectVersionArgument(D, Args)) {
+    auto CodeObjArg = StringRef(getAMDGPUCodeObjectArgument(D, Args)->getValue());
+    if(CodeObjArg.starts_with("none"))  return TargetOptions::COV_None;
+    CodeObjArg.getAsInteger(0, CodeObjVer);
+  }
   return CodeObjVer;
 }
 
 bool tools::haveAMDGPUCodeObjectVersionArgument(
     const Driver &D, const llvm::opt::ArgList &Args) {
   return getAMDGPUCodeObjectArgument(D, Args) != nullptr;
 }