[RISCV] Match GCC `-march`/`-mabi` driver defaults

[lenary]

Summary

Clang/LLVM is a cross-compiler, and so we don't have to make a choice
about -march/-mabi at build-time, but we may have to compute a
default -march/-mabi when compiling a program. Until now, each
place that has needed a default -march has calculated one itself.

This patch adds a single place where a default -march is calculated,
in order to avoid calculating different defaults in different places.

This patch adds a new function riscv::getRISCVArch which encapsulates
this logic based on GCC's for computing a default -march value
when none is provided. This patch also updates the logic in
riscv::getRISCVABI to match the logic in GCC's build system for
computing a default -mabi.

This patch also updates anywhere that -march is used to now use the
new function which can compute a default. In particular, we now
explicitly pass a -march value down to the gnu assembler.

GCC has convoluted logic in its build system to choose a default
-march/-mabi based on build options, which would be good to match.
This patch is based on the logic in GCC 9.2.0. This commit's logic is
different to GCC's only for baremetal targets, where we default
to rv32imac/ilp32 or rv64imac/lp64 depending on the target triple.

Tests have been updated to match the new logic.

[lenary]

• Correct code formatting issue

[rogfer01]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

475

llvm::StringSwitch has a method StartsWithLower which might help make the logic a bit clearer

Something like this (I haven't tested it!)

StringRef ABIFromMarch = StringSwitch(MArch)
   .StartsWithLower("rv32d", "ilp32d")
   .StartsWithLower("rv32g", "ilp32d")
   .StartsWithLower("rv32e", "ilp32e")
   .StartsWithLower("rv32", "ilp32")

   .StartsWithLower("rv64d", "lp64d")
   .StartsWithLower("rv64g", "lp64d")
   .StartsWithLower("rv64", "lp64").

   .Default("");

if (!ABIFromMarch.empty()) return ABIFromMarch;

[lenary]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

475

Sadly I don't think this will work, because of the case of matching rv32*d* and rv64*d* (the March.substr(4).contains_lower("d") cases) from config.gcc. Explicitly "d" does not come immediately after rv<32/64>, it can come anywhere after like in rv32imafdc.

The other issue I have with the StringSwitch is that it requires I have a default, which I feel makes the control flow harder to understand, rather than easier.

[khchen]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

537

Why do you set rv32gc and rv64gc as default march? Is there a any reason?

[lenary]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

537

This reflects the logic in [[ https://github.com/riscv/riscv-gcc/blob/riscv-gcc-9.2.0/gcc/config.gcc#L4229-L4369 | config.gcc ]]. Note line 4273, which applies if neither arch nor abi is given (the xlen is set on line 4233-4234 based on the target triple).

[rogfer01]

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

475

Oh I see.

Then I would comment what this part does with a bit more detail right after the // 2. Choose a default based on -march=. For example

// rv32g | rv32*d -> ilp32d
// rv32e -> ilp32e
// rv32* -> ilp32
// rv64g | rv64*d -> lp64d
// rv64* -> lp64

Given that gcc is using a shell glob, then GlobPattern in Support/GlobPattern.h may help as well. But it might be overkill in this scenario.

[simoncook]

I have a question about backwards compatibility with this patch. Clang 9 has shipped with rvXXg/etc defaulting to ilp32/lp64 ABI, and no march meaning rvXXi, with users having built objects with those defaults. When Clang 10 ships, users they now need to always use a mabi/march flag to keep the same compatibility with their Clang 9 flows, the enabled extensions and ABI will be changed under their feet without warning (or at least until they either hit a linker error in the ABI case, or potentially an invalid instruction trap in the march case).

If we're going to change the defaults, this patch should at least contain an update to the release notes file, this way this change would be documented for users.

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

476

Won’t this break if the user specifies a X/Z extension that has a d in the name, so eg rv32iXd will try to use the ilp32d abi by default? For future proofing, I think we may need to do a full parse of the isa string to verify that d does actually mean the standard D-extension

[lenary]

I agree backwards compatibility is hard here.

• This method was introduced to have a single place to choose a default march string if none was chosen before. I think this change is useful (saves defaults being calculated in a multitude of other places, which means they may not agree).
• The choice of a default march string was based, as closely as possible, on config.gcc. This may not be what we want (and is the source of backwards compatibility issues).
• I 100% agree that a release notes entry is needed. I will write one just as soon as we finalize the default behaviour.
• The elf multilib changes in D67508 seem to be more complex, as config.gcc chooses a march/mabi default that is not built by t-elf-multilib.

I think I would understand updating the logic to use fewer architecture extensions for a given ABI (i.e., default to rv32i unless someone asks for ilp32f/d) rather than more (rv32gc when someone asks for ilp32). This should be more backwards compatible, but also requires carefully ensuring the logic is not circular.

At the very least, I do not plan to merge this patch until we have had a chance to discuss it next Thursday (31st Oct) on the RISC-V backend call.

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

476

You're right that having "d" in any extension name will cause issues. The same is true for any gcc build today (9.2.0).

[lenary]

• More conservative march/mabi defaults on riscv-unknown-elf
• Add clang release notes about this change to -march/-mabi

[lenary]

• Update tests to reflect new defaults

[khchen]

LGTM, thanks for the patch!

[lenary]

• Rebase
• Update comments

[lenary]

I think this is just about ready to land, given our discussion on the RISC-V call on 7 November 2019.

[luismarques]

LGTM.

[asb]

Please update the commit message to clarify the cases where we do deviate from the GCC defaults, but this looks good to me.

[lenary]

Rebase and update comments and release notes

clang/lib/Driver/ToolChains/Arch/RISCV.h

 namespace driver {
 namespace tools {
 namespace riscv {
 void getRISCVTargetFeatures(const Driver &D, const llvm::Triple &Triple,
                             const llvm::opt::ArgList &Args,
                             std::vector<llvm::StringRef> &Features);
 StringRef getRISCVABI(const llvm::opt::ArgList &Args,
                       const llvm::Triple &Triple);
+StringRef getRISCVArch(const llvm::opt::ArgList &Args,
+                       const llvm::Triple &Triple);
 } // end namespace riscv
 } // namespace tools
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_ARCH_RISCV_H

clang/lib/Driver/ToolChains/Arch/RISCV.cpp

static bool getArchFeatures(const Driver &D, StringRef MArch,
   getExtensionFeatures(D, Args, Features, MArch, OtherExts);
 
   return true;
 }
 
 void riscv::getRISCVTargetFeatures(const Driver &D, const llvm::Triple &Triple,
                                    const ArgList &Args,
                                    std::vector<StringRef> &Features) {
-  llvm::Optional<StringRef> MArch;
-  if (const Arg *A = Args.getLastArg(options::OPT_march_EQ))
-    MArch = A->getValue();
-  else if (Triple.getOS() == llvm::Triple::Linux)
-    // RISC-V Linux defaults to rv{32,64}gc.
-    MArch = Triple.getArch() == llvm::Triple::riscv32 ? "rv32gc" : "rv64gc";
+  StringRef MArch = getRISCVArch(Args, Triple);
 
-  if (MArch.hasValue() && !getArchFeatures(D, *MArch, Features, Args))
+  if (!getArchFeatures(D, MArch, Features, Args))
     return;
 
   // Handle features corresponding to "-ffixed-X" options
   if (Args.hasArg(options::OPT_ffixed_x1))
     Features.push_back("+reserve-x1");
   if (Args.hasArg(options::OPT_ffixed_x2))
     Features.push_back("+reserve-x2");
   if (Args.hasArg(options::OPT_ffixed_x3))
void riscv::getRISCVTargetFeatures(const Driver &D, const llvm::Triple &Triple,
   handleTargetFeaturesGroup(Args, Features, options::OPT_m_riscv_Features_Group);
 }
 
 StringRef riscv::getRISCVABI(const ArgList &Args, const llvm::Triple &Triple) {
   assert((Triple.getArch() == llvm::Triple::riscv32 ||
           Triple.getArch() == llvm::Triple::riscv64) &&
          "Unexpected triple");
 
+  // GCC's logic around choosing a default `-mabi=` is complex. If GCC is not
+  // configured using `--with-abi=`, then the logic for the default choice is
+  // defined in config.gcc.
+  //
+  // This logic uses the following, in order:
+  // 1. Explicit choices using `--with-abi=`
+  // 2. A default based on `--with-arch=`, if provided
+  // 3. A default based on the target triple's arch
+  //
+  // This logic in config.gcc is a little circular but it is not inconsistent.
+  //
+  // Clang does not have `--with-arch=` or `--with-abi=`, so we use `-march=`
+  // and `-mabi=` respectively instead.
+
+  // 1. If `-mabi=` is specified, use it.
   if (const Arg *A = Args.getLastArg(options::OPT_mabi_EQ))
     return A->getValue();
 
-  // RISC-V Linux defaults to ilp32d/lp64d
-  if (Triple.getOS() == llvm::Triple::Linux)
-    return Triple.getArch() == llvm::Triple::riscv32 ? "ilp32d" : "lp64d";
+  // 2. Choose a default based on `-march=`
+  if (const Arg *A = Args.getLastArg(options::OPT_march_EQ)) {
+    StringRef MArch = A->getValue();
+
+    if (MArch.startswith_lower("rv32")) {
+      if (MArch.substr(4).contains_lower("d") ||
+          MArch.startswith_lower("rv32g"))
+        return "ilp32d";
+      else if (MArch.startswith_lower("rv32e"))
+        return "ilp32e";
+      else
+        return "ilp32";
+    } else if (MArch.startswith_lower("rv64")) {
+      if (MArch.substr(4).contains_lower("d") ||
+          MArch.startswith_lower("rv64g"))
+        return "lp64d";
+      else
+        return "lp64";
+    }
+  }
+
+  // 3. Choose a default based on the triple
+  if (Triple.getArch() == llvm::Triple::riscv32)
+    return "ilp32d";
+  else
+    return "lp64d";
+}
+
+StringRef riscv::getRISCVArch(const llvm::opt::ArgList &Args,
+                              const llvm::Triple &Triple) {
+  assert((Triple.getArch() == llvm::Triple::riscv32 ||
+          Triple.getArch() == llvm::Triple::riscv64) &&
+         "Unexpected triple");
+
+  // GCC's logic around choosing a default `-march=` is complex. If GCC is not
+  // configured using `--with-arch=`, then the logic for the default choice is
+  // defined in config.gcc.
+  //
+  // This logic uses the following, in order:
+  // 1. Explicit choices using `--with-arch=`
+  // 2. A default based on `--with-abi=`, if provided
+  // 3. A default based on the target triple's arch
+  //
+  // This logic in config.gcc is a little circular but it is not inconsistent.
+  //
+  // Clang does not have `--with-arch=` or `--with-abi=`, so we use `-march=`
+  // and `-mabi=` respectively instead.
+
+  // 1. If `-march=` is specified, use it.
+  if (const Arg *A = Args.getLastArg(options::OPT_march_EQ))
+    return A->getValue();
+
+  // 2. Choose a default based on `-mabi=`
+  if (const Arg *A = Args.getLastArg(options::OPT_mabi_EQ)) {
+    StringRef MABI = A->getValue();
+
+    if (MABI.equals_lower("ilp32e"))
+      return "rv32e";
+    else if (MABI.startswith_lower("ilp32"))
+      return "rv32gc";
+    else if (MABI.startswith_lower("lp64"))
+      return "rv64gc";
+  }
+
+  // 3. Choose a default based on the triple
+  if (Triple.getArch() == llvm::Triple::riscv32)
+    return "rv32gc";
   else
-    return Triple.getArch() == llvm::Triple::riscv32 ? "ilp32" : "lp64";
+    return "rv64gc";
 }

clang/lib/Driver/ToolChains/Gnu.cpp

CmdArgs.push_back(
       ppc::getPPCAsmModeForCPU(getCPUName(Args, getToolChain().getTriple())));
     break;
   }
   case llvm::Triple::riscv32:
   case llvm::Triple::riscv64: {
     StringRef ABIName = riscv::getRISCVABI(Args, getToolChain().getTriple());
     CmdArgs.push_back("-mabi");
     CmdArgs.push_back(ABIName.data());
-    if (const Arg *A = Args.getLastArg(options::OPT_march_EQ)) {
-      StringRef MArch = A->getValue();
-      CmdArgs.push_back("-march");
-      CmdArgs.push_back(MArch.data());
-    }
+    StringRef MArchName = riscv::getRISCVArch(Args, getToolChain().getTriple());
+    CmdArgs.push_back(MArchName.data());
     break;
   }
   case llvm::Triple::sparc:
   case llvm::Triple::sparcel: {
     CmdArgs.push_back("-32");
     std::string CPU = getCPUName(Args, getToolChain().getTriple());
     CmdArgs.push_back(
         sparc::getSparcAsmModeForCPU(CPU, getToolChain().getTriple()));

clang/test/Driver/riscv-abi.c

-// RUN: %clang -target riscv32-unknown-elf %s -### -o %t.o 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHECK-ILP32 %s
 // RUN: %clang -target riscv32-unknown-elf %s -### -o %t.o -mabi=ilp32 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ILP32 %s
-// RUN: %clang -target riscv32-unknown-elf -x assembler %s -### -o %t.o 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHECK-ILP32 %s
 // RUN: %clang -target riscv32-unknown-elf -x assembler %s -### -o %t.o \
 // RUN:   -mabi=ilp32 2>&1 | FileCheck -check-prefix=CHECK-ILP32 %s
 
 // CHECK-ILP32: "-target-abi" "ilp32"
 
 // RUN: %clang -target riscv32-unknown-elf %s -### -o %t.o -march=rv32if -mabi=ilp32f 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ILP32F %s
 
 // CHECK-ILP32F: "-target-abi" "ilp32f"
 
+// RUN: %clang -target riscv32-unknown-elf %s -### -o %t.o 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ILP32D %s
+// RUN: %clang -target riscv32-unknown-elf -x assembler %s -### -o %t.o 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ILP32D %s
 // RUN: %clang -target riscv32-unknown-elf %s -### -o %t.o -march=rv32ifd -mabi=ilp32d 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ILP32D %s
 
 // CHECK-ILP32D: "-target-abi" "ilp32d"
 
 // RUN: not %clang -target riscv32-unknown-elf %s -o %t.o -mabi=lp64 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-RV32-LP64 %s
 
 // CHECK-RV32-LP64: error: unknown target ABI 'lp64'
 
-// RUN: %clang -target riscv64-unknown-elf %s -### -o %t.o 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHECK-LP64 %s
 // RUN: %clang -target riscv64-unknown-elf %s -### -o %t.o -mabi=lp64 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64 %s
-// RUN: %clang -target riscv64-unknown-elf -x assembler %s -### -o %t.o 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHECK-LP64  %s
 // RUN: %clang -target riscv64-unknown-elf -x assembler %s -### -o %t.o \
 // RUN:   -mabi=lp64 2>&1 | FileCheck -check-prefix=CHECK-LP64 %s
 
 // CHECK-LP64: "-target-abi" "lp64"
 
 // RUN:  %clang -target riscv64-unknown-elf %s -### -o %t.o -march=rv64f -mabi=lp64f 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64F %s
 
 // CHECK-LP64F: "-target-abi" "lp64f"
 
+// RUN: %clang -target riscv64-unknown-elf %s -### -o %t.o 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-LP64D %s
+// RUN: %clang -target riscv64-unknown-elf -x assembler %s -### -o %t.o 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-LP64D  %s
 // RUN: %clang -target riscv64-unknown-elf %s -### -o %t.o -march=rv64d -mabi=lp64d 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64D %s
 
 // CHECK-LP64D: "-target-abi" "lp64d"
 
 // RUN: not %clang -target riscv64-unknown-elf %s -o %t.o -mabi=ilp32 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-RV64-ILP32 %s
 
 // CHECK-RV64-ILP32: error: unknown target ABI 'ilp32'

clang/test/Driver/riscv-gnutools.c

 // Check gnutools are invoked with propagated values for -mabi and -march.
 
 // RUN: %clang -target riscv32 -fno-integrated-as %s -###  -c \
-// RUN: 2>&1 | FileCheck -check-prefix=MABI-ILP32 %s
+// RUN: 2>&1 | FileCheck -check-prefix=MABI-ILP32D %s
 
 // RUN: %clang -target riscv32 -fno-integrated-as -march=rv32g %s -### -c \
-// RUN: 2>&1 | FileCheck -check-prefix=MABI-ILP32-MARCH-G %s
+// RUN: 2>&1 | FileCheck -check-prefix=MABI-ILP32D-MARCH-G %s
 
 // RUN: %clang -target riscv64 -fno-integrated-as %s -###  -c \
-// RUN: 2>&1 | FileCheck -check-prefix=MABI-ILP64 %s
+// RUN: 2>&1 | FileCheck -check-prefix=MABI-ILP64D %s
 
 // RUN: %clang -target riscv64 -fno-integrated-as -march=rv64g %s -### -c \
-// RUN: 2>&1 | FileCheck -check-prefix=MABI-ILP64-MARCH-G %s
+// RUN: 2>&1 | FileCheck -check-prefix=MABI-ILP64D-MARCH-G %s
 
-// MABI-ILP32: "{{.*}}as{{(.exe)?}}" "-mabi" "ilp32"
-// MABI-ILP32-MARCH-G: "{{.*}}as{{(.exe)?}}" "-mabi" "ilp32" "-march" "rv32g"
+// MABI-ILP32D: "{{.*}}as{{(.exe)?}}" "-mabi" "ilp32d" "-march" "rv32gc"
+// MABI-ILP32D-MARCH-G: "{{.*}}as{{(.exe)?}}" "-mabi" "ilp32d" "-march" "rv32g"
 
-// MABI-ILP64: "{{.*}}as{{(.exe)?}}" "-mabi" "lp64"
-// MABI-ILP64-MARCH-G: "{{.*}}as{{(.exe)?}}" "-mabi" "lp64" "-march" "rv64g"
+// MABI-ILP64D: "{{.*}}as{{(.exe)?}}" "-mabi" "lp64d"
+// MABI-ILP64D-MARCH-G: "{{.*}}as{{(.exe)?}}" "-mabi" "lp64d" "-march" "rv64g"