[RISCV] Set triple based on -march flag which can be deduced in more generic way

[zixuan-wu]

Summary

Now RISC-V the value provided to -march can determine whether to compile for 32- or 64-bit RISC-V irrespective of the target provided to the Clang driver. It was introduced at https://reviews.llvm.org/D54214 before.

So this patch tries to go further based on D54214 that -march can be deduced in more generic way without providing -march but -mcpu or -mabi. To make it more clear.

Before:
clang -target=riscv32-- -march=rv64gc, riscv32 will be replaced by riscv64 to avoid report error.
After:
clang -target=riscv32-- -mcpu=sifive-s21, riscv32 will also be replaced by riscv64 because sifive-s21' default march is rv64imac provided at RISCVTargetParser.def.

[jrtc27]

Does GCC allow this or not? Because this strikes me as a bad idea at first sight…

[zixuan-wu]

In D129824#3653895, @jrtc27 wrote:

Does GCC allow this or not? Because this strikes me as a bad idea at first sight…

GCC can deduce from -mcpu but not -mabi.

riscv64-unknown-linux-gnu-gcc a.c -mabi=ilp32
cc1: error: ABI requires '-march=rv32'

But I think it can be negotiated.

[eopXD]

clang/lib/Driver/Driver.cpp

659

Do we need to throw error (or warning) when these two (RVArch and ArchName) don't match?

[zixuan-wu]

clang/lib/Driver/Driver.cpp

659

It just overrides it as before to not report error when for example default triple is rv64 with user passing -march=rv32xxx

[zixuan-wu]

Remove unnecessary include header.

[reames]

This was very briefly discussed at today's sync up call. We were running short on time, so we didn't get a chance to talk through it, but there did seem to be a consensus that discussion on the interface implications was needed. This should hopefully be on the agenda when we talk again in two weeks.

[zixuan-wu]

In D129824#3670586, @reames wrote:

This was very briefly discussed at today's sync up call. We were running short on time, so we didn't get a chance to talk through it, but there did seem to be a consensus that discussion on the interface implications was needed. This should hopefully be on the agenda when we talk again in two weeks.

Okay. Anybody else please more comments here before next sync-up call if you have.

[jrtc27]

If you still want to pursue this, discussion belongs at https://github.com/riscv-non-isa/riscv-toolchain-conventions, not here, since it's an interface shared by Clang and GCC and the two should be consistent

[kito-cheng]

Just realized the problem is trying to fixed the default value of -mabi=, currently clang -target riscv32-elf -march=rv64gc -mabi=lp64d/riscv32-elf-clang -march=rv64gc -mabi=lp64d is work, and match the behavior of GCC did, riscv32-elf-gcc -march=rv64gc -mabi=lp64d.

And this patch is trying to make following two command work: clang -target riscv32-elf -march=rv64gc/riscv32-elf-clang -march=rv64gc, specify -march and -target but no -mabi.

That is different story now, GCC isn't deduce the default abi from either target triple or abi, so if you invoke gcc with riscv32-elf-gcc -march=rv64gc or riscv64-elf-gcc -march=rv32gc, you will got error message like that: cc1: error: ABI requires '-march=rv32' or cc1: error: ABI requires '-march=rv64'.

So that's not compatible issue with GCC, that's sort of clang driver specify issue, and I don't have strong opinion on this.

---

As a GNU toolchain developer, I would say, we are not intend to change the behavior of default value of -mabi or -march, the consensus among RISC-V GNU toolchain maintainer is user should explicitly specify the -march and -mabi if you are not using default -march and -mabi.

[• pcwang-thead]

In D129824#3699911, @kito-cheng wrote:

Just realized the problem is trying to fixed the default value of -mabi=, currently clang -target riscv32-elf -march=rv64gc -mabi=lp64d/riscv32-elf-clang -march=rv64gc -mabi=lp64d is work, and match the behavior of GCC did, riscv32-elf-gcc -march=rv64gc -mabi=lp64d.

And this patch is trying to make following two command work: clang -target riscv32-elf -march=rv64gc/riscv32-elf-clang -march=rv64gc, specify -march and -target but no -mabi.

That is different story now, GCC isn't deduce the default abi from either target triple or abi, so if you invoke gcc with riscv32-elf-gcc -march=rv64gc or riscv64-elf-gcc -march=rv32gc, you will got error message like that: cc1: error: ABI requires '-march=rv32' or cc1: error: ABI requires '-march=rv64'.

So that's not compatible issue with GCC, that's sort of clang driver specify issue, and I don't have strong opinion on this.

---

As a GNU toolchain developer, I would say, we are not intend to change the behavior of default value of -mabi or -march, the consensus among RISC-V GNU toolchain maintainer is user should explicitly specify the -march and -mabi if you are not using default -march and -mabi.

@kito-cheng Thanks! We have noticed these differences between GNU and LLVM, and we have done some efforts to make both compatible.
I think we can discuss here: https://github.com/riscv-non-isa/riscv-toolchain-conventions/issues/20

[zixuan-wu]

In D129824#3699911, @kito-cheng wrote:

Just realized the problem is trying to fixed the default value of -mabi=, currently clang -target riscv32-elf -march=rv64gc -mabi=lp64d/riscv32-elf-clang -march=rv64gc -mabi=lp64d is work, and match the behavior of GCC did, riscv32-elf-gcc -march=rv64gc -mabi=lp64d.

And this patch is trying to make following two command work: clang -target riscv32-elf -march=rv64gc/riscv32-elf-clang -march=rv64gc, specify -march and -target but no -mabi.

That is different story now, GCC isn't deduce the default abi from either target triple or abi, so if you invoke gcc with riscv32-elf-gcc -march=rv64gc or riscv64-elf-gcc -march=rv32gc, you will got error message like that: cc1: error: ABI requires '-march=rv32' or cc1: error: ABI requires '-march=rv64'.

So that's not compatible issue with GCC, that's sort of clang driver specify issue, and I don't have strong opinion on this.

---

As a GNU toolchain developer, I would say, we are not intend to change the behavior of default value of -mabi or -march, the consensus among RISC-V GNU toolchain maintainer is user should explicitly specify the -march and -mabi if you are not using default -march and -mabi.

This patch is not about default value of mabi. It just tries to deduce march when -march option is not provided explicitly.
It fixes the conflict between target triple and march about 32-bit/64-bit (riscv32 or riscv64) issue as before. Before this patch, this conflict only can be solved by specifying -march option explicitly.

[asb]

@zixuan-wu we discussed this a bit in the last community sync call. One aspect that makes this patch a little hard to review is the lack of a clear patch description summarising the behaviour before the patch, the behaviour after the patch, and the motivation for the change. I appreciate that the time isn't very convenient in all timezones, but if you were able to attend the next call (18th August, same time) to discuss, that might really help agree a path forwards on this change.

[zixuan-wu]

In D129824#3706654, @asb wrote:

@zixuan-wu we discussed this a bit in the last community sync call. One aspect that makes this patch a little hard to review is the lack of a clear patch description summarising the behaviour before the patch, the behaviour after the patch, and the motivation for the change. I appreciate that the time isn't very convenient in all timezones, but if you were able to attend the next call (18th August, same time) to discuss, that might really help agree a path forwards on this change.

I updated the description. I think it can be understood after see D54214.

[jrtc27]

clang/test/Driver/riscv-arch.c

410

This one maybe makes sense to allow, though gets murky once you have cores that support writable XLEN as then the same CPU exists for both RV32 and RV64 (like how you can ask for say haswell i386 and x86_64) so I'm not entirely sure...

414

IMO this kind of thing should remain an error, you're asking for an ABI that doesn't exist for the requested architecture

[zixuan-wu]

clang/test/Driver/riscv-arch.c

410

For now, if the name of sifive-s21 can't be both RV32 and RV64. It must distinguish the name in RISCVTargetParser.def.

414

IMO, if -march is accepted to override riscv32 target triple, and march can get by riscv::getRISCVArch function with -mabi, then it should also accept. Or riscv::getRISCVArch should not get -march from -mabi.

[zixuan-wu]

ping...

[MaskRay]

Both D54214 and this look like a surprising behavior to me. Do we still have time to revert D54214 and make mismatching --target & -march= an error?

clang -target=riscv32-- -march=rv64gc

-target= => --target=

For --target=riscv64 -march=rv64i -m32, -march= appears to take precedence, but the rule seems confusing.

[jrtc27]

In D129824#3794221, @MaskRay wrote:

Both D54214 and this look like a surprising behavior to me. Do we still have time to go back the state before D54214 and make mismatching --target & -march= an error?

Then there's no -m32 equivalent; that's what -march currently gives you... also GCC lets you do it. And -target is Clang-specific so you can't write something that works for both compilers.

[MaskRay]

In D129824#3794229, @jrtc27 wrote:

In D129824#3794221, @MaskRay wrote:

Both D54214 and this look like a surprising behavior to me. Do we still have time to go back the state before D54214 and make mismatching --target & -march= an error?

Then there's no -m32 equivalent; that's what -march currently gives you... also GCC lets you do it. And -target is Clang-specific so you can't write something that works for both compilers.

We can accept -m32/-m64 as a special case. Other -m options

(I edited my previous comment and added a confusing --target=riscv64 -march=rv64i -m32 case when you made the comment.)

[zixuan-wu]

AFAIK, --target is clang-specific and transparent for compiler user in RV side. -m is also undefined or less used by RV user. It just uses -march to specify 32 or 64 mode and extensions.
Is the convention specified in RV Spec?

[zixuan-wu]

Ping.. Do we still prefer to specify target triple explicitly to make a check?

[zixuan-wu]

As this issue is solved at https://reviews.llvm.org/D148124, abandon this revision.

clang/lib/Driver/Driver.cpp

 //===--- Driver.cpp - Clang GCC Compatible Driver -------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Driver/Driver.h"
+#include "ToolChains/Arch/RISCV.h"
 #include "ToolChains/AIX.h"
 #include "ToolChains/AMDGPU.h"
 #include "ToolChains/AMDGPUOpenMP.h"
 #include "ToolChains/AVR.h"
 #include "ToolChains/Ananas.h"
 #include "ToolChains/BareMetal.h"
 #include "ToolChains/CSKYToolChain.h"
 #include "ToolChains/Clang.h"
if (ABIName == "32") {
       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_insensitive("rv32"))
+  // provided architecture relative info such as -march/-mcpu/-mabi.
+  if (Target.isRISCV()) {
+    StringRef ArchName = tools::riscv::getRISCVArch(Args, Target);
+    llvm::Triple::ArchType RVArch = Target.getArch();
+
+    if (RVArch == llvm::Triple::riscv64 &&
+        ArchName.startswith_insensitive("rv32"))
       Target.setArch(llvm::Triple::riscv32);
-    else if (ArchName.startswith_insensitive("rv64"))
+    else if (RVArch == llvm::Triple::riscv32 &&
+             ArchName.startswith_insensitive("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(=.*)? or -f(no-)?offload-lto(=.*)?

clang/test/Driver/riscv-abi.c

 // RUN:   | FileCheck -check-prefix=CHECK-ILP32D %s
 // RUN: %clang --target=riscv32-unknown-linux-gnu %s -### 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ILP32D %s
 // RUN: %clang --target=riscv32-unknown-linux-gnu -x assembler %s -### 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 -### 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64 %s
 // RUN: %clang --target=riscv64-unknown-elf %s -### -mabi=lp64 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64 %s
 // RUN: %clang --target=riscv64-unknown-elf %s -### -march=rv64imc 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64 %s
 // RUN: %clang --target=riscv64-unknown-elf %s -### -march=rv64imf 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64 %s

 // RUN: %clang --target=riscv64-unknown-elf %s -### -march=rv64g 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64D %s
 // RUN: %clang --target=riscv64-unknown-linux-gnu %s -### 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64D %s
 // RUN: %clang --target=riscv64-unknown-linux-gnu -x assembler %s -### 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-arch.c

 // 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
+// RUN: %clang --target=riscv32-unknown-elf -mcpu=sifive-e20 -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-TARGET %s
+// RUN: %clang --target=riscv64-unknown-elf -mcpu=sifive-e20 -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-TARGET %s
+// RUN: %clang --target=riscv32-unknown-elf -mabi=ilp32 -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-TARGET %s
+// RUN: %clang --target=riscv64-unknown-elf -mabi=ilp32 -### %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
+// RUN: %clang --target=riscv32-unknown-elf -mcpu=sifive-s21 -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64-TARGET %s
+// RUN: %clang --target=riscv64-unknown-elf -mcpu=sifive-s21 -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64-TARGET %s
+// RUN: %clang --target=riscv32-unknown-elf -mabi=lp64 -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64-TARGET %s
+// RUN: %clang --target=riscv64-unknown-elf -mabi=lp64 -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64-TARGET %s
 // RV64-TARGET: "-triple" "riscv64-unknown-unknown-elf"
 
 // RUN: %clang --target=riscv32-unknown-elf -march=rv32ifzfh01p0 -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-ZFH %s
 // RUN: %clang --target=riscv32-unknown-elf -march=rv32ifzfh -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-ZFH %s
 // RV32-ZFH: "-target-feature" "+zfh"