[clang][RISCV] Change default abi with f extension but without d extension #73489

jacquesguan · 2023-11-27T08:40:59Z

Now we have default abi lp64 for rv64if and ilp32 for rv32if, which is different with riscv-gnu-toolchain. In https://github.com/riscv-collab/riscv-gnu-toolchain/blob/8e9fb09a0c4b1e566492ee6f42e8c1fa5ef7e0c2/configure#L3385 when have f and not d, it prefers lp64f/ilp32f but no soft float. This patch tries to make their behaviors consistent.

llvmbot · 2023-11-27T08:41:31Z

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-risc-v

Author: Jianjian Guan (jacquesguan)

Changes

Now we have default abi lp64 for rv64if and ilp32 for rv32if, which is different with riscv-gnu-toolchain. In https://github.com/riscv-collab/riscv-gnu-toolchain/blob/8e9fb09a0c4b1e566492ee6f42e8c1fa5ef7e0c2/configure#L3385 when have f and not d, it prefers lp64f/ilp32f but no soft float. This patch tries to make their behaviors consistent.

Full diff: https://github.com/llvm/llvm-project/pull/73489.diff

2 Files Affected:

(modified) clang/test/Driver/riscv-abi.c (+9-5)
(modified) llvm/lib/Support/RISCVISAInfo.cpp (+4)

diff --git a/clang/test/Driver/riscv-abi.c b/clang/test/Driver/riscv-abi.c
index e67f790e0de0e59..16568271564c797 100644
--- a/clang/test/Driver/riscv-abi.c
+++ b/clang/test/Driver/riscv-abi.c
@@ -4,8 +4,6 @@
 // RUN:   | FileCheck -check-prefix=CHECK-ILP32 %s
 // RUN: %clang --target=riscv32-unknown-elf %s -### -march=rv32imc 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ILP32 %s
-// RUN: %clang --target=riscv32-unknown-elf %s -### -march=rv32imf 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHECK-ILP32 %s
 // RUN: %clang --target=riscv32-unknown-elf -x assembler %s -### 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ILP32 %s
 // RUN: %clang --target=riscv32-unknown-elf -x assembler %s -### \
@@ -24,6 +22,10 @@
 
 // RUN: %clang --target=riscv32-unknown-elf %s -### -march=rv32if -mabi=ilp32f 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ILP32F %s
+// RUN: %clang --target=riscv32-unknown-elf %s -### -mabi=ilp32f 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ILP32F %s
+// RUN: %clang --target=riscv32-unknown-elf %s -### -march=rv32if 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-ILP32F %s
 
 // CHECK-ILP32F: "-target-abi" "ilp32f"
 
@@ -51,8 +53,6 @@
 // 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 -x assembler %s -### 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64  %s
 // RUN: %clang --target=riscv64-unknown-elf -x assembler %s -### \
@@ -60,7 +60,11 @@
 
 // CHECK-LP64: "-target-abi" "lp64"
 
-// RUN:  not %clang --target=riscv64-unknown-elf %s -### -march=rv64f -mabi=lp64f 2>&1 \
+// RUN:  %clang --target=riscv64-unknown-elf %s -### -march=rv64if -mabi=lp64f 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-LP64F %s
+// RUN:  %clang --target=riscv64-unknown-elf %s -### -mabi=lp64f 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-LP64F %s
+// RUN:  %clang --target=riscv64-unknown-elf %s -### -march=rv64if 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-LP64F %s
 
 // CHECK-LP64F: "-target-abi" "lp64f"
diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index 6322748430063ce..24d0d40cbc74ebc 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -1292,12 +1292,16 @@ StringRef RISCVISAInfo::computeDefaultABI() const {
   if (XLen == 32) {
     if (hasExtension("d"))
       return "ilp32d";
+    if (hasExtension("f"))
+      return "ilp32f";
     if (hasExtension("e"))
       return "ilp32e";
     return "ilp32";
   } else if (XLen == 64) {
     if (hasExtension("d"))
       return "lp64d";
+    if (hasExtension("f"))
+      return "lp64f";
     if (hasExtension("e"))
       return "lp64e";
     return "lp64";

wangpc-pp

I have similar patch before: https://reviews.llvm.org/D125947, so it LGTM as GCC's default behavior has changed. :-)
But please wait for others' opinions.

kito-cheng · 2023-11-28T15:56:59Z

short version: GCC isn't change.
long version: GCC's configure script isn't change, it's configure script in riscv-gnu-toolchain

But I don't have strong opinion on this change since I believe user should explicitly specify that, otherwise it's really to screw up to select multi-lib or portability issue between different compilers.

wangpc-pp · 2023-11-28T16:17:03Z

short version: GCC isn't change. long version: GCC's configure script isn't change, it's configure script in riscv-gnu-toolchain

So why is there a difference between GCC and riscv-gnu-toolchain? If we set with_abi to lp64f, what is the behavior?

But I don't have strong opinion on this change since I believe user should explicitly specify that, otherwise it's really to screw up to select multi-lib or portability issue between different compilers.

jacquesguan · 2023-12-05T09:20:17Z

soft ping... Any more advice?

MaskRay · 2023-12-06T02:53:20Z

clang/test/Driver/riscv-cpus.c

@@ -1,4 +1,4 @@
-// Check target CPUs are correctly passed.
+·// Check target CPUs are correctly passed.


Stray .? Please fix

MaskRay · 2023-12-06T02:53:34Z

when only have f extension but no d extension

This can be simplified to "w/ f extension but w/o d extension"

benshi001 · 2023-12-07T08:38:36Z

I think this is reasonable, even I myself made such a change months ago in Phabracator, but finally not approved.

wangpc-pp · 2023-12-07T08:46:14Z

As @kito-cheng has pointed out, we should fix multilib issue before landing this. It seems we don't generate multilib for ilp32f and lp64f in riscv-gnu-toolchain?

asb · 2023-12-07T14:32:02Z

This is a user-facing change that definitely needs to be acknowledged in the release notes (clang/docs/ReleaseNotes.rst)

I agree with you that it seems more intuitive that a -march=rv32imaf invocation should default to ilp32f just as -march=rv32imafd defaults to ilp32d. I slightly disagree that this patch aligns us to GCC, because as I understood Kito's comment the GCC behaviour is really depending on what default ABI the toolchain was configured with. For a toolchain that defaults to ilp32d, presumably -march=rv32imaf still ends up with ilp32?

That said, with the GCC cross-compilation model being so different to clang I don't think we necessarily need to match precisely, so I'm not opposed to a more intuitive default.

asb · 2023-12-07T18:21:57Z

I think the conclusion from the LLVM sync-up call was that everyone happy to move in this direction, so please add the release note and we can do a final review. Thanks!

jacquesguan · 2023-12-08T09:45:23Z

I think the conclusion from the LLVM sync-up call was that everyone happy to move in this direction, so please add the release note and we can do a final review. Thanks!

Done, added release note.

asb · 2023-12-08T17:22:24Z

I think the conclusion from the LLVM sync-up call was that everyone happy to move in this direction, so please add the release note and we can do a final review. Thanks!

Done, added release note.

Thanks! Sorry I wasn't specific about this, but we need a Clang release note as well.

…nsion Now we have default abi lp64 for rv64if and ilp32 for rv32if, which is different with riscv-gnu-toolchain. In https://github.com/riscv-collab/riscv-gnu-toolchain/blob/8e9fb09a0c4b1e566492ee6f42e8c1fa5ef7e0c2/configure#L3385 when have f but not, it prefers lp64f/ilp32f but no soft float. This patch tries to make their behaviors consistent.

jacquesguan · 2023-12-09T02:11:50Z

I think the conclusion from the LLVM sync-up call was that everyone happy to move in this direction, so please add the release note and we can do a final review. Thanks!

Done, added release note.

Thanks! Sorry I wasn't specific about this, but we need a Clang release note as well.

Done, added Clang release note too.

asb

LGTM

jacquesguan requested review from MaskRay, zakk0610, kito-cheng, topperc and benshi001 November 27, 2023 08:40

llvmbot added clang Clang issues not falling into any other category backend:RISC-V clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' llvm:support labels Nov 27, 2023

jacquesguan force-pushed the default_abi_with_only_f branch from b41c0a9 to e03da61 Compare November 27, 2023 09:16

wangpc-pp approved these changes Nov 27, 2023

View reviewed changes

jacquesguan force-pushed the default_abi_with_only_f branch from e03da61 to daaa69a Compare November 28, 2023 03:28

MaskRay reviewed Dec 6, 2023

View reviewed changes

MaskRay approved these changes Dec 6, 2023

View reviewed changes

jacquesguan force-pushed the default_abi_with_only_f branch from daaa69a to ce22351 Compare December 7, 2023 03:04

jacquesguan changed the title ~~[clang][RISCV] Change default abi when only have f extension but no d extension~~ [clang][RISCV] Change default abi with f extension but without d extension Dec 7, 2023

benshi001 approved these changes Dec 7, 2023

View reviewed changes

jacquesguan requested review from asb and lukel97 December 7, 2023 08:42

jacquesguan requested review from 4vtomat and yetingk December 7, 2023 09:07

jacquesguan force-pushed the default_abi_with_only_f branch from ce22351 to dcc7164 Compare December 8, 2023 09:44

jacquesguan force-pushed the default_abi_with_only_f branch from dcc7164 to 4e54425 Compare December 9, 2023 02:08

asb approved these changes Dec 13, 2023

View reviewed changes

jacquesguan merged commit 3fe8141 into llvm:main Dec 15, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang][RISCV] Change default abi with f extension but without d extension #73489

[clang][RISCV] Change default abi with f extension but without d extension #73489

jacquesguan commented Nov 27, 2023

llvmbot commented Nov 27, 2023 •

edited

wangpc-pp left a comment

kito-cheng commented Nov 28, 2023

wangpc-pp commented Nov 28, 2023

jacquesguan commented Dec 5, 2023

MaskRay Dec 6, 2023

jacquesguan Dec 7, 2023

MaskRay commented Dec 6, 2023

benshi001 commented Dec 7, 2023

wangpc-pp commented Dec 7, 2023

asb commented Dec 7, 2023

asb commented Dec 7, 2023

jacquesguan commented Dec 8, 2023

asb commented Dec 8, 2023

jacquesguan commented Dec 9, 2023

asb left a comment

		@@ -1,4 +1,4 @@
		// Check target CPUs are correctly passed.
		·// Check target CPUs are correctly passed.

[clang][RISCV] Change default abi with f extension but without d extension #73489

[clang][RISCV] Change default abi with f extension but without d extension #73489

Conversation

jacquesguan commented Nov 27, 2023

llvmbot commented Nov 27, 2023 • edited

wangpc-pp left a comment

Choose a reason for hiding this comment

kito-cheng commented Nov 28, 2023

wangpc-pp commented Nov 28, 2023

jacquesguan commented Dec 5, 2023

MaskRay Dec 6, 2023

Choose a reason for hiding this comment

jacquesguan Dec 7, 2023

Choose a reason for hiding this comment

MaskRay commented Dec 6, 2023

benshi001 commented Dec 7, 2023

wangpc-pp commented Dec 7, 2023

asb commented Dec 7, 2023

asb commented Dec 7, 2023

jacquesguan commented Dec 8, 2023

asb commented Dec 8, 2023

jacquesguan commented Dec 9, 2023

asb left a comment

Choose a reason for hiding this comment

llvmbot commented Nov 27, 2023 •

edited