[RISCV] Set triple based on -march flag

[simoncook]

Summary

For RISC-V the value provided to -march should determine whether to compile for 32- or 64-bit RISC-V irrespective of the target provided to the Clang driver. This adds a test for this flag for RISC-V and sets the Target architecture correctly in these cases.

[simoncook]

Rebase on top of tree.

I've noticed that without this flag calling riscv32-unknown-elf-clang -march=rv64i will still produce a rv32 binary, which is unlike GCC's behavior

[lenary]

What happens if I pass clang -march=rv32i -target riscv64-unknown-elf? Should we care about the ordering of -march vs -target?

[simoncook]

In D54214#1609967, @lenary wrote:

What happens if I pass clang -march=rv32i -target riscv64-unknown-elf? Should we care about the ordering of -march vs -target?

Currently this patch makes the output be generated for rv32i. I think that -march should override -target since it is more fine grained, is used as the default way of controlling extensions, and may be less confusing than last XLEN wins.

[simoncook]

Ping, before I rebased this did anyone have any other thoughts on flag precedence?

[jrtc27]

In D54214#1718043, @simoncook wrote:

Ping, before I rebased this did anyone have any other thoughts on flag precedence?

The precedence seems sensible; it's what MIPS is doing for -mabi vs -target just above.

clang/lib/Driver/Driver.cpp

541 ↗

(On Diff #212772)

Missing a full-stop here

[lenary]

I have no issue with the underlying logic here.

Please rebase on top of D69383 and use riscv::getRISCVArch(...) defined in that patch rather than querying Args.getLastArg(options::OPT_march_EQ) directly.

[simoncook]

Rebase.

@lenary Following the discussion regarding D69383, I think it's best for now to keep the logic just keeping -march directly, rather than using getRISCVArch. I think in the case of -target risc32-..... -mabi=lp64 I think it would confuse users if the tools suddenly changed to doing an rv64 compile. If we disable that, all that function would provide me is the same StringRef I'm already evaluating. I think adding any extra flag to indicate whether a rv32<->rv64 switch is acceptable would just make the code unnecessarily more messy. I think in the future if getRISCVArch evaluates more flags, then it might make sense to reconsider this.

[lenary]

In D54214#1737255, @simoncook wrote:

Rebase.

@lenary Following the discussion regarding D69383, I think it's best for now to keep the logic just keeping -march directly, ...

Yep, I understand.

Ok, Logic and tests LGTM. I think this needs an entry in the Release Notes too.

[zixuan-wu]

In D54214#1737255, @simoncook wrote:

Rebase.

@lenary Following the discussion regarding D69383, I think it's best for now to keep the logic just keeping -march directly, rather than using getRISCVArch. I think in the case of -target risc32-..... -mabi=lp64 I think it would confuse users if the tools suddenly changed to doing an rv64 compile. If we disable that, all that function would provide me is the same StringRef I'm already evaluating. I think adding any extra flag to indicate whether a rv32<->rv64 switch is acceptable would just make the code unnecessarily more messy. I think in the future if getRISCVArch evaluates more flags, then it might make sense to reconsider this.

I think the case of -target risc32-..... -mabi=lp64 is acceptable since the -target is less usable than -march/-mabi/-mcpu and is almost invisible to users.

lib/Driver/Driver.cpp

if (A && Target.isMIPS()) {
     } else if (ABIName == "64") {
       Target = Target.get64BitArchVariant();
       if (Target.getEnvironment() == llvm::Triple::GNU ||
           Target.getEnvironment() == llvm::Triple::GNUABIN32)
         Target.setEnvironment(llvm::Triple::GNUABI64);
     }
   }
 
+  // If target is RISC-V adjust the target triple according to
+  // provided architecture name
+  A = Args.getLastArg(options::OPT_march_EQ);
+  if (A && Target.isRISCV()) {
+    StringRef ArchName = A->getValue();
+    if (ArchName.startswith_lower("rv32"))
+      Target.setArch(llvm::Triple::riscv32);
+    else if (ArchName.startswith_lower("rv64"))
+      Target.setArch(llvm::Triple::riscv64);
+  }
+
   return Target;
 }
 
 // Parse the LTO options and record the type of LTO compilation
 // based on which -f(no-)?lto(=.*)? option occurs last.
 void Driver::setLTOMode(const llvm::opt::ArgList &Args) {
   LTOMode = LTOK_None;
   if (!Args.hasFlag(options::OPT_flto, options::OPT_flto_EQ,

test/Driver/riscv-arch.c

 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-X-X-INVAL %s
 // RV32-X-X-INVAL: error: invalid arch name 'rv32ixabc_xdef', unsupported
 // RV32-X-X-INVAL: non-standard user-level extension 'xabc'
 
 // RUN: %clang -target riscv32-unknown-elf -march=rv32ixabc_sdef_sxghi -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-X-S-SX-INVAL %s
 // RV32-X-S-SX-INVAL: error: invalid arch name 'rv32ixabc_sdef_sxghi',
 // RV32-X-S-SX-INVAL: unsupported non-standard user-level extension 'xabc'
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv32i -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-TARGET %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv32i -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-TARGET %s
+// RV32-TARGET: "-triple" "riscv32-unknown-unknown-elf"
+
+// RUN: %clang -target riscv32-unknown-elf -march=rv64i -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64-TARGET %s
+// RUN: %clang -target riscv64-unknown-elf -march=rv64i -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64-TARGET %s
+// RV64-TARGET: "-triple" "riscv64-unknown-unknown-elf"