Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISCV] Use the 'B' extension in RISC-V profile definitions #113942

Merged
merged 4 commits into from
Nov 10, 2024

Conversation

asb
Copy link
Contributor

@asb asb commented Oct 28, 2024

RVA22 has retroactively been defined as including 'B' (as it's a
shorthand for Zba+Zbb+Zbs, which were previously explicitly enumerated)
and RV{A,B,M}23 are defined featuring B. We don't currently infer B
whenever Zba+Zbb+Zbs are present due to concerns about compatibility
with external assemblers such as gas.

We don't believe that adding B to RVA22 will cause issues for users who (for instance) build with clang and assemble with binutils as looking at the binutils commit history:
zic64b support was only committed in 25f05199bb7e35820c23e802424484accb7936b1 in July 2024
B support was committed in c144f638337944101131d9fe6de4ab908f6d4c2d in May 2024

So given we emit zic64b anyway (as it has always been in the RVA22 spec), no binutils that would have previously successfully assembled our rva22u64 output should fail due to the addition of 'B'.


Note: This was initially posted as an RFC but is now ready for review

asb added 2 commits October 28, 2024 14:42
RVA22 has retrospectively been defined as including 'B' (as it's a
shorthand for Zba+Zbb+Zbs, which were previously explicitly enumerated)
and RV{A,B,M}23 are defined featuring B. We don't currently infer B
whenever Zba+Zbb+Zbs are present due to concerns about compatibility
with external assemblers such as gas.
@llvmbot 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' labels Oct 28, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 28, 2024

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

@llvm/pr-subscribers-clang-driver

Author: Alex Bradbury (asb)

Changes

Stacks on top of #113918.

RVA22 has retroactively been defined as including 'B' (as it's a
shorthand for Zba+Zbb+Zbs, which were previously explicitly enumerated)
and RV{A,B,M}23 are defined featuring B. We don't currently infer B
whenever Zba+Zbb+Zbs are present due to concerns about compatibility
with external assemblers such as gas.

Marked as an RFC as it seems worth checking we're happy with the implications of this, especially when it comes to changing RVA22. e.g. are we happy it won't confuse external tools, and do we forsee any issues from the canonical normalised ISA naming string for RVA22 now being different to what it was. I've listed this on the agenda for the next RISC-V LLVM sync call.

Note: RVM23 doesn't currently use B, but I filed riscv/riscv-profiles#191 which I'd expect would be accepted.


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

7 Files Affected:

  • (modified) clang/test/Driver/print-supported-extensions-riscv.c (+2-2)
  • (modified) clang/test/Driver/riscv-profiles.c (+2-2)
  • (modified) llvm/docs/RISCVUsage.rst (+2-2)
  • (modified) llvm/docs/ReleaseNotes.md (+2-1)
  • (modified) llvm/lib/Target/RISCV/RISCVProfiles.td (+5-11)
  • (modified) llvm/test/CodeGen/RISCV/attributes.ll (+9-9)
  • (modified) llvm/unittests/TargetParser/RISCVISAInfoTest.cpp (+2-2)
diff --git a/clang/test/Driver/print-supported-extensions-riscv.c b/clang/test/Driver/print-supported-extensions-riscv.c
index e39847b9c31a8e..68acde65a74bfb 100644
--- a/clang/test/Driver/print-supported-extensions-riscv.c
+++ b/clang/test/Driver/print-supported-extensions-riscv.c
@@ -193,12 +193,12 @@
 // CHECK-NEXT:     rva22u64
 // CHECK-NEXT:     rva23s64
 // CHECK-NEXT:     rva23u64
+// CHECK-NEXT:     rvb23s64
+// CHECK-NEXT:     rvb23u64
 // CHECK-NEXT:     rvi20u32
 // CHECK-NEXT:     rvi20u64
 // CHECK-EMPTY:
 // CHECK-NEXT: Experimental Profiles
-// CHECK-NEXT:     rvb23s64
-// CHECK-NEXT:     rvb23u64
 // CHECK-NEXT:     rvm23u32
 // CHECK-EMPTY:
 // CHECK-NEXT: Use -march to specify the target's extension.
diff --git a/clang/test/Driver/riscv-profiles.c b/clang/test/Driver/riscv-profiles.c
index d85ac8baf4edd9..2b4d19422874cf 100644
--- a/clang/test/Driver/riscv-profiles.c
+++ b/clang/test/Driver/riscv-profiles.c
@@ -207,7 +207,7 @@
 // RVA23S64: "-target-feature" "+svnapot"
 // RVA23S64: "-target-feature" "+svpbmt"
 
-// RUN: %clang --target=riscv64 -### -c %s 2>&1 -march=rvb23u64 -menable-experimental-extensions \
+// RUN: %clang --target=riscv64 -### -c %s 2>&1 -march=rvb23u64 \
 // RUN:   | FileCheck -check-prefix=RVB23U64 %s
 // RVB23U64: "-target-feature" "+m"
 // RVB23U64: "-target-feature" "+a"
@@ -239,7 +239,7 @@
 // RVB23U64: "-target-feature" "+zbs"
 // RVB23U64: "-target-feature" "+zkt"
 
-// RUN: %clang --target=riscv64 -### -c %s 2>&1 -march=rvb23s64 -menable-experimental-extensions \
+// RUN: %clang --target=riscv64 -### -c %s 2>&1 -march=rvb23s64 \
 // RUN:   | FileCheck -check-prefix=RVB23S64 %s
 // RVB23S64: "-target-feature" "+m"
 // RVB23S64: "-target-feature" "+a"
diff --git a/llvm/docs/RISCVUsage.rst b/llvm/docs/RISCVUsage.rst
index 04f2c357766d44..f6f2eb45c49c17 100644
--- a/llvm/docs/RISCVUsage.rst
+++ b/llvm/docs/RISCVUsage.rst
@@ -84,6 +84,8 @@ ISA naming string. Currently supported profiles:
 * ``rva22s64``
 * ``rva23u64``
 * ``rva23s64``
+* ``rvb23u64``
+* ``rvb23s64``
 
 Note that you can also append additional extension names to be enabled, e.g.
 ``rva20u64_zicond`` will enable the ``zicond`` extension in addition to those
@@ -93,8 +95,6 @@ Profiles that are not yet ratified cannot be used unless
 ``-menable-experimental-extensions`` (or equivalent for other tools) is
 specified. This applies to the following profiles:
 
-* ``rvb23u64``
-* ``rvb23s64``
 * ``rvm23u32``
 
 .. _riscv-extensions:
diff --git a/llvm/docs/ReleaseNotes.md b/llvm/docs/ReleaseNotes.md
index ac7a795daf791a..92a45d845f1db8 100644
--- a/llvm/docs/ReleaseNotes.md
+++ b/llvm/docs/ReleaseNotes.md
@@ -184,7 +184,8 @@ Changes to the RISC-V Backend
 * The `Smmpm`, `Smnpm`, `Ssnpm`, `Supm`, and `Sspm` pointer masking extensions
   are no longer marked as experimental.
 * The `Sha` extension is now supported.
-* The RVA23U64 and RVA23S64 profiles are no longer marked as experimental.
+* The RVA23U64, RVA23S64, RVB23U64, and RVB23S64 profiles are no longer marked
+  as experimental.
 
 Changes to the WebAssembly Backend
 ----------------------------------
diff --git a/llvm/lib/Target/RISCV/RISCVProfiles.td b/llvm/lib/Target/RISCV/RISCVProfiles.td
index ea0fe08abd7a14..bcb776e682aea7 100644
--- a/llvm/lib/Target/RISCV/RISCVProfiles.td
+++ b/llvm/lib/Target/RISCV/RISCVProfiles.td
@@ -45,9 +45,7 @@ defvar RVA22U64Features = !listconcat(RVA20U64BaseFeatures,
                                       [FeatureStdExtZa64rs,
                                        FeatureStdExtZihpm,
                                        FeatureStdExtZihintpause,
-                                       FeatureStdExtZba,
-                                       FeatureStdExtZbb,
-                                       FeatureStdExtZbs,
+                                       FeatureStdExtB,
                                        FeatureStdExtZic64b,
                                        FeatureStdExtZicbom,
                                        FeatureStdExtZicbop,
@@ -92,9 +90,7 @@ defvar RVB23U64Features = !listconcat(RVA20U64BaseFeatures,
                                       [FeatureStdExtZihpm,
                                        FeatureStdExtZa64rs,
                                        FeatureStdExtZihintpause,
-                                       FeatureStdExtZba,
-                                       FeatureStdExtZbb,
-                                       FeatureStdExtZbs,
+                                       FeatureStdExtB,
                                        FeatureStdExtZic64b,
                                        FeatureStdExtZicbom,
                                        FeatureStdExtZicbop,
@@ -128,9 +124,7 @@ defvar RVB23S64Features = !listconcat(RVB23U64Features,
 defvar RVM23U32Features = [Feature32Bit,
                            FeatureStdExtI,
                            FeatureStdExtM,
-                           FeatureStdExtZba,
-                           FeatureStdExtZbb,
-                           FeatureStdExtZbs,
+                           FeatureStdExtB,
                            FeatureStdExtZicond,
                            FeatureStdExtZihintpause,
                            FeatureStdExtZihintntl,
@@ -163,6 +157,6 @@ def RVA22U64 : RISCVProfile<"rva22u64", RVA22U64Features>;
 def RVA22S64 : RISCVProfile<"rva22s64", RVA22S64Features>;
 def RVA23U64 : RISCVProfile<"rva23u64", RVA23U64Features>;
 def RVA23S64 : RISCVProfile<"rva23s64", RVA23S64Features>;
-def RVB23U64 : RISCVExperimentalProfile<"rvb23u64", RVB23U64Features>;
-def RVB23S64 : RISCVExperimentalProfile<"rvb23s64", RVB23S64Features>;
+def RVB23U64 : RISCVProfile<"rvb23u64", RVB23U64Features>;
+def RVB23S64 : RISCVProfile<"rvb23s64", RVB23S64Features>;
 def RVM23U32 : RISCVExperimentalProfile<"rvm23u32", RVM23U32Features>;
diff --git a/llvm/test/CodeGen/RISCV/attributes.ll b/llvm/test/CodeGen/RISCV/attributes.ll
index 2545c7075e4cf5..8688009f492b94 100644
--- a/llvm/test/CodeGen/RISCV/attributes.ll
+++ b/llvm/test/CodeGen/RISCV/attributes.ll
@@ -293,8 +293,8 @@
 ; RUN: llc -mtriple=riscv64 -mattr=+rva22s64 %s -o - | FileCheck --check-prefix=RVA22S64 %s
 ; RUN: llc -mtriple=riscv64 -mattr=+rva23u64 %s -o - | FileCheck --check-prefix=RVA23U64 %s
 ; RUN: llc -mtriple=riscv64 -mattr=+rva23s64 %s -o - | FileCheck --check-prefix=RVA23S64 %s
-; RUN: llc -mtriple=riscv64 -mattr=+experimental-rvb23u64 %s -o - | FileCheck --check-prefix=RVB23U64 %s
-; RUN: llc -mtriple=riscv64 -mattr=+experimental-rvb23s64 %s -o - | FileCheck --check-prefix=RVB23S64 %s
+; RUN: llc -mtriple=riscv64 -mattr=+rvb23u64 %s -o - | FileCheck --check-prefix=RVB23U64 %s
+; RUN: llc -mtriple=riscv64 -mattr=+rvb23s64 %s -o - | FileCheck --check-prefix=RVB23S64 %s
 ; RUN: llc -mtriple=riscv32 -mattr=+experimental-rvm23u32 %s -o - | FileCheck --check-prefix=RVM23U32 %s
 
 ; CHECK: .attribute 4, 16
@@ -584,13 +584,13 @@
 ; RVI20U64: .attribute 5, "rv64i2p1"
 ; RVA20U64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicsr2p0_zmmul1p0_za128rs1p0"
 ; RVA20S64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicsr2p0_zifencei2p0_zmmul1p0_za128rs1p0_ssccptr1p0_sstvala1p0_sstvecd1p0_svade1p0_svbare1p0"
-; RVA22U64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_zic64b1p0_zicbom1p0_zicbop1p0_zicboz1p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicsr2p0_zihintpause2p0_zihpm2p0_zmmul1p0_za64rs1p0_zfhmin1p0_zba1p0_zbb1p0_zbs1p0_zkt1p0"
-; RVA22S64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_zic64b1p0_zicbom1p0_zicbop1p0_zicboz1p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicsr2p0_zifencei2p0_zihintpause2p0_zihpm2p0_zmmul1p0_za64rs1p0_zfhmin1p0_zba1p0_zbb1p0_zbs1p0_zkt1p0_ssccptr1p0_sscounterenw1p0_sstvala1p0_sstvecd1p0_svade1p0_svbare1p0_svinval1p0_svpbmt1p0"
-; RVA23U64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_v1p0_zic64b1p0_zicbom1p0_zicbop1p0_zicboz1p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicond1p0_zicsr2p0_zihintntl1p0_zihintpause2p0_zihpm2p0_zimop1p0_zmmul1p0_za64rs1p0_zawrs1p0_zfa1p0_zfhmin1p0_zca1p0_zcb1p0_zcmop1p0_zba1p0_zbb1p0_zbs1p0_zkt1p0_zvbb1p0_zve32f1p0_zve32x1p0_zve64d1p0_zve64f1p0_zve64x1p0_zvfhmin1p0_zvkb1p0_zvkt1p0_zvl128b1p0_zvl32b1p0_zvl64b1p0_supm1p0"
-; RVA23S64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_v1p0_h1p0_zic64b1p0_zicbom1p0_zicbop1p0_zicboz1p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicond1p0_zicsr2p0_zifencei2p0_zihintntl1p0_zihintpause2p0_zihpm2p0_zimop1p0_zmmul1p0_za64rs1p0_zawrs1p0_zfa1p0_zfhmin1p0_zca1p0_zcb1p0_zcmop1p0_zba1p0_zbb1p0_zbs1p0_zkt1p0_zvbb1p0_zve32f1p0_zve32x1p0_zve64d1p0_zve64f1p0_zve64x1p0_zvfhmin1p0_zvkb1p0_zvkt1p0_zvl128b1p0_zvl32b1p0_zvl64b1p0_sha1p0_shcounterenw1p0_shgatpa1p0_shtvala1p0_shvsatpa1p0_shvstvala1p0_shvstvecd1p0_ssccptr1p0_sscofpmf1p0_sscounterenw1p0_ssnpm1p0_ssstateen1p0_sstc1p0_sstvala1p0_sstvecd1p0_ssu64xl1p0_supm1p0_svade1p0_svbare1p0_svinval1p0_svnapot1p0_svpbmt1p0"
-; RVB23U64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_zic64b1p0_zicbom1p0_zicbop1p0_zicboz1p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicond1p0_zicsr2p0_zihintntl1p0_zihintpause2p0_zihpm2p0_zimop1p0_zmmul1p0_za64rs1p0_zawrs1p0_zfa1p0_zca1p0_zcb1p0_zcmop1p0_zba1p0_zbb1p0_zbs1p0_zkt1p0"
-; RVB23S64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_zic64b1p0_zicbom1p0_zicbop1p0_zicboz1p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicond1p0_zicsr2p0_zifencei2p0_zihintntl1p0_zihintpause2p0_zihpm2p0_zimop1p0_zmmul1p0_za64rs1p0_zawrs1p0_zfa1p0_zca1p0_zcb1p0_zcmop1p0_zba1p0_zbb1p0_zbs1p0_zkt1p0_ssccptr1p0_sscofpmf1p0_sscounterenw1p0_sstc1p0_sstvala1p0_sstvecd1p0_ssu64xl1p0_svade1p0_svbare1p0_svinval1p0_svnapot1p0_svpbmt1p0"
-; RVM23U32: .attribute 5, "rv32i2p1_m2p0_zicbop1p0_zicond1p0_zicsr2p0_zihintntl1p0_zihintpause2p0_zimop1p0_zmmul1p0_zca1p0_zcb1p0_zce1p0_zcmop1p0_zcmp1p0_zcmt1p0_zba1p0_zbb1p0_zbs1p0"
+; RVA22U64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_b1p0_zic64b1p0_zicbom1p0_zicbop1p0_zicboz1p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicsr2p0_zihintpause2p0_zihpm2p0_zmmul1p0_za64rs1p0_zfhmin1p0_zba1p0_zbb1p0_zbs1p0_zkt1p0"
+; RVA22S64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_b1p0_zic64b1p0_zicbom1p0_zicbop1p0_zicboz1p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicsr2p0_zifencei2p0_zihintpause2p0_zihpm2p0_zmmul1p0_za64rs1p0_zfhmin1p0_zba1p0_zbb1p0_zbs1p0_zkt1p0_ssccptr1p0_sscounterenw1p0_sstvala1p0_sstvecd1p0_svade1p0_svbare1p0_svinval1p0_svpbmt1p0"
+; RVA23U64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_b1p0_v1p0_zic64b1p0_zicbom1p0_zicbop1p0_zicboz1p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicond1p0_zicsr2p0_zihintntl1p0_zihintpause2p0_zihpm2p0_zimop1p0_zmmul1p0_za64rs1p0_zawrs1p0_zfa1p0_zfhmin1p0_zca1p0_zcb1p0_zcmop1p0_zba1p0_zbb1p0_zbs1p0_zkt1p0_zvbb1p0_zve32f1p0_zve32x1p0_zve64d1p0_zve64f1p0_zve64x1p0_zvfhmin1p0_zvkb1p0_zvkt1p0_zvl128b1p0_zvl32b1p0_zvl64b1p0_supm1p0"
+; RVA23S64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_b1p0_v1p0_h1p0_zic64b1p0_zicbom1p0_zicbop1p0_zicboz1p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicond1p0_zicsr2p0_zifencei2p0_zihintntl1p0_zihintpause2p0_zihpm2p0_zimop1p0_zmmul1p0_za64rs1p0_zawrs1p0_zfa1p0_zfhmin1p0_zca1p0_zcb1p0_zcmop1p0_zba1p0_zbb1p0_zbs1p0_zkt1p0_zvbb1p0_zve32f1p0_zve32x1p0_zve64d1p0_zve64f1p0_zve64x1p0_zvfhmin1p0_zvkb1p0_zvkt1p0_zvl128b1p0_zvl32b1p0_zvl64b1p0_sha1p0_shcounterenw1p0_shgatpa1p0_shtvala1p0_shvsatpa1p0_shvstvala1p0_shvstvecd1p0_ssccptr1p0_sscofpmf1p0_sscounterenw1p0_ssnpm1p0_ssstateen1p0_sstc1p0_sstvala1p0_sstvecd1p0_ssu64xl1p0_supm1p0_svade1p0_svbare1p0_svinval1p0_svnapot1p0_svpbmt1p0"
+; RVB23U64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_b1p0_zic64b1p0_zicbom1p0_zicbop1p0_zicboz1p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicond1p0_zicsr2p0_zihintntl1p0_zihintpause2p0_zihpm2p0_zimop1p0_zmmul1p0_za64rs1p0_zawrs1p0_zfa1p0_zca1p0_zcb1p0_zcmop1p0_zba1p0_zbb1p0_zbs1p0_zkt1p0"
+; RVB23S64: .attribute 5, "rv64i2p1_m2p0_a2p1_f2p2_d2p2_c2p0_b1p0_zic64b1p0_zicbom1p0_zicbop1p0_zicboz1p0_ziccamoa1p0_ziccif1p0_zicclsm1p0_ziccrse1p0_zicntr2p0_zicond1p0_zicsr2p0_zifencei2p0_zihintntl1p0_zihintpause2p0_zihpm2p0_zimop1p0_zmmul1p0_za64rs1p0_zawrs1p0_zfa1p0_zca1p0_zcb1p0_zcmop1p0_zba1p0_zbb1p0_zbs1p0_zkt1p0_ssccptr1p0_sscofpmf1p0_sscounterenw1p0_sstc1p0_sstvala1p0_sstvecd1p0_ssu64xl1p0_svade1p0_svbare1p0_svinval1p0_svnapot1p0_svpbmt1p0"
+; RVM23U32: .attribute 5, "rv32i2p1_m2p0_b1p0_zicbop1p0_zicond1p0_zicsr2p0_zihintntl1p0_zihintpause2p0_zimop1p0_zmmul1p0_zca1p0_zcb1p0_zce1p0_zcmop1p0_zcmp1p0_zcmt1p0_zba1p0_zbb1p0_zbs1p0"
 
 define i32 @addi(i32 %a) {
   %1 = add i32 %a, 1
diff --git a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
index 48792ad0265fc4..a1d493e12fda6d 100644
--- a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
@@ -1138,12 +1138,12 @@ Supported Profiles
     rva22u64
     rva23s64
     rva23u64
+    rvb23s64
+    rvb23u64
     rvi20u32
     rvi20u64
 
 Experimental Profiles
-    rvb23s64
-    rvb23u64
     rvm23u32
 
 Use -march to specify the target's extension.

@lenary
Copy link
Member

lenary commented Oct 29, 2024

I think we shouldn't do this for RVA22, so as not to break existing users of that profile, who may have a toolchain that doesn't support B.

This change makes sense to me for the RV*23 profiles, especially since your change to RVM23 has been accepted.

@topperc
Copy link
Collaborator

topperc commented Oct 29, 2024

The change should be reflected in the -target-feature checks in clang/test/Driver/riscv-profiles.c too.

@asb
Copy link
Contributor Author

asb commented Nov 7, 2024

I think we shouldn't do this for RVA22, so as not to break existing users of that profile, who may have a toolchain that doesn't support B.

This change makes sense to me for the RV*23 profiles, especially since your change to RVM23 has been accepted.

For RVA22, I had a look at the commit history for extension support in binutils. From what I can see (@kito-cheng please correct me if I'm wrong):

  • zic64b support was only committed in 25f05199bb7e35820c23e802424484accb7936b1 in July 2024
  • B support was committed in c144f638337944101131d9fe6de4ab908f6d4c2d in May 2024

So given we emit zic64b anyway (as it has always been in the RVA22 spec), I think we probably do have freedom to add in B here without breaking binutils that would have previously successfully assembled our rva22u64 output.

@lenary
Copy link
Member

lenary commented Nov 7, 2024

I think we shouldn't do this for RVA22, so as not to break existing users of that profile, who may have a toolchain that doesn't support B.
This change makes sense to me for the RV*23 profiles, especially since your change to RVM23 has been accepted.

For RVA22, I had a look at the commit history for extension support in binutils. From what I can see (@kito-cheng please correct me if I'm wrong):

  • zic64b support was only committed in 25f05199bb7e35820c23e802424484accb7936b1 in July 2024
  • B support was committed in c144f638337944101131d9fe6de4ab908f6d4c2d in May 2024

So given we emit zic64b anyway (as it has always been in the RVA22 spec), I think we probably do have freedom to add in B here without breaking binutils that would have previously successfully assembled our rva22u64 output.

Alright, then I don't have any worries about RVA22.

@asb asb changed the title [RFC][RISCV] Use the 'B' extension in RISC-V profile definitions [RISCV] Use the 'B' extension in RISC-V profile definitions Nov 9, 2024
@asb
Copy link
Contributor Author

asb commented Nov 9, 2024

I've added the CHECK lines to riscv-profiles.c as spotted by @topperc, and based on the above discussion I think this is now ready to review.

Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@asb asb merged commit e149528 into llvm:main Nov 10, 2024
8 checks passed
@llvm-ci
Copy link
Collaborator

llvm-ci commented Nov 10, 2024

LLVM Buildbot has detected a new failure on builder sanitizer-x86_64-linux-bootstrap-asan running on sanitizer-buildbot2 while building clang,llvm at step 2 "annotate".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/52/builds/3611

Here is the relevant piece of the build log for the reference
Step 2 (annotate) failure: 'python ../sanitizer_buildbot/sanitizers/zorg/buildbot/builders/sanitizers/buildbot_selector.py' (failure)
...
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using lld-link: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/lld-link
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using ld64.lld: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/ld64.lld
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using wasm-ld: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/wasm-ld
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using ld.lld: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/ld.lld
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using lld-link: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/lld-link
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using ld64.lld: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/ld64.lld
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using wasm-ld: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/wasm-ld
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/main.py:72: note: The test suite configuration requested an individual test timeout of 0 seconds but a timeout of 900 seconds was requested on the command line. Forcing timeout to be 900 seconds.
-- Testing: 87036 of 87037 tests, 88 workers --
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.
FAIL: lld :: ELF/gdb-index-multiple-cu.s (85022 of 87036)
******************** TEST 'lld :: ELF/gdb-index-multiple-cu.s' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 2: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/llvm-mc -filetype=obj -triple=x86_64-unknown-linux /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/lld/test/ELF/gdb-index-multiple-cu.s -o /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/tools/lld/test/ELF/Output/gdb-index-multiple-cu.s.tmp.o
+ /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/llvm-mc -filetype=obj -triple=x86_64-unknown-linux /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/lld/test/ELF/gdb-index-multiple-cu.s -o /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/tools/lld/test/ELF/Output/gdb-index-multiple-cu.s.tmp.o
RUN: at line 3: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/ld.lld --gdb-index /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/tools/lld/test/ELF/Output/gdb-index-multiple-cu.s.tmp.o -o /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/tools/lld/test/ELF/Output/gdb-index-multiple-cu.s.tmp 2>&1 | /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/count 0
+ /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/ld.lld --gdb-index /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/tools/lld/test/ELF/Output/gdb-index-multiple-cu.s.tmp.o -o /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/tools/lld/test/ELF/Output/gdb-index-multiple-cu.s.tmp
+ /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/count 0
Expected 0 lines, got 1.

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
Slowest Tests:
--------------------------------------------------------------------------
259.64s: LLVM :: CodeGen/AMDGPU/sched-group-barrier-pipeline-solver.mir
111.63s: Clang :: Preprocessor/riscv-target-features.c
96.57s: Clang :: OpenMP/target_update_codegen.cpp
94.80s: Clang :: OpenMP/target_defaultmap_codegen_01.cpp
91.63s: Clang :: Driver/fsanitize.c
79.80s: Clang :: Preprocessor/arm-target-features.c
78.08s: Clang :: Preprocessor/aarch64-target-features.c
77.41s: Clang :: Driver/arm-cortex-cpus-2.c
75.77s: Clang :: Driver/arm-cortex-cpus-1.c
72.21s: Clang :: Preprocessor/predefined-arch-macros.c
69.00s: LLVM :: CodeGen/RISCV/attributes.ll
68.12s: LLVM :: CodeGen/AMDGPU/memintrinsic-unroll.ll
63.60s: Clang :: Analysis/a_flaky_crash.cpp
57.65s: Clang :: CodeGen/aarch64-sve-intrinsics/acle_sve_reinterpret.c
53.58s: Clang :: Driver/clang_f_opts.c
52.21s: Clang :: Driver/linux-ld.c
50.30s: Clang :: CodeGen/aarch64-sve-intrinsics/acle_sve_reinterpret-bfloat.c
48.48s: LLVM :: CodeGen/ARM/build-attributes.ll
48.09s: Clang :: Driver/cl-options.c
46.75s: Clang :: Preprocessor/predefined-macros-no-warnings.c
Step 10 (stage2/asan check) failure: stage2/asan check (failure)
...
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using lld-link: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/lld-link
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using ld64.lld: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/ld64.lld
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using wasm-ld: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/wasm-ld
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using ld.lld: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/ld.lld
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using lld-link: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/lld-link
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using ld64.lld: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/ld64.lld
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/llvm/config.py:506: note: using wasm-ld: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/wasm-ld
llvm-lit: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/utils/lit/lit/main.py:72: note: The test suite configuration requested an individual test timeout of 0 seconds but a timeout of 900 seconds was requested on the command line. Forcing timeout to be 900 seconds.
-- Testing: 87036 of 87037 tests, 88 workers --
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.
FAIL: lld :: ELF/gdb-index-multiple-cu.s (85022 of 87036)
******************** TEST 'lld :: ELF/gdb-index-multiple-cu.s' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 2: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/llvm-mc -filetype=obj -triple=x86_64-unknown-linux /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/lld/test/ELF/gdb-index-multiple-cu.s -o /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/tools/lld/test/ELF/Output/gdb-index-multiple-cu.s.tmp.o
+ /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/llvm-mc -filetype=obj -triple=x86_64-unknown-linux /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/lld/test/ELF/gdb-index-multiple-cu.s -o /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/tools/lld/test/ELF/Output/gdb-index-multiple-cu.s.tmp.o
RUN: at line 3: /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/ld.lld --gdb-index /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/tools/lld/test/ELF/Output/gdb-index-multiple-cu.s.tmp.o -o /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/tools/lld/test/ELF/Output/gdb-index-multiple-cu.s.tmp 2>&1 | /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/count 0
+ /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/ld.lld --gdb-index /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/tools/lld/test/ELF/Output/gdb-index-multiple-cu.s.tmp.o -o /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/tools/lld/test/ELF/Output/gdb-index-multiple-cu.s.tmp
+ /home/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/count 0
Expected 0 lines, got 1.

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
Slowest Tests:
--------------------------------------------------------------------------
259.64s: LLVM :: CodeGen/AMDGPU/sched-group-barrier-pipeline-solver.mir
111.63s: Clang :: Preprocessor/riscv-target-features.c
96.57s: Clang :: OpenMP/target_update_codegen.cpp
94.80s: Clang :: OpenMP/target_defaultmap_codegen_01.cpp
91.63s: Clang :: Driver/fsanitize.c
79.80s: Clang :: Preprocessor/arm-target-features.c
78.08s: Clang :: Preprocessor/aarch64-target-features.c
77.41s: Clang :: Driver/arm-cortex-cpus-2.c
75.77s: Clang :: Driver/arm-cortex-cpus-1.c
72.21s: Clang :: Preprocessor/predefined-arch-macros.c
69.00s: LLVM :: CodeGen/RISCV/attributes.ll
68.12s: LLVM :: CodeGen/AMDGPU/memintrinsic-unroll.ll
63.60s: Clang :: Analysis/a_flaky_crash.cpp
57.65s: Clang :: CodeGen/aarch64-sve-intrinsics/acle_sve_reinterpret.c
53.58s: Clang :: Driver/clang_f_opts.c
52.21s: Clang :: Driver/linux-ld.c
50.30s: Clang :: CodeGen/aarch64-sve-intrinsics/acle_sve_reinterpret-bfloat.c
48.48s: LLVM :: CodeGen/ARM/build-attributes.ll
48.09s: Clang :: Driver/cl-options.c
46.75s: Clang :: Preprocessor/predefined-macros-no-warnings.c

@lenary
Copy link
Member

lenary commented Nov 11, 2024

Sorry I didn't get to this over the weekend. LGTM.

Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
)

RVA22 has retroactively been defined as including 'B' (as it's a
shorthand for Zba+Zbb+Zbs, which were previously explicitly enumerated)
and RV{A,B,M}23 are defined featuring B. We don't currently infer B
whenever Zba+Zbb+Zbs are present due to concerns about compatibility
with external assemblers such as gas.

We don't believe that adding B to RVA22 will cause issues for users who
(for instance) build with clang and assemble with binutils as looking at
the binutils commit history:
zic64b support was only committed in
25f05199bb7e35820c23e802424484accb7936b1 in July 2024
B support was committed in c144f638337944101131d9fe6de4ab908f6d4c2d in
May 2024

So given we emit zic64b anyway (as it has always been in the RVA22
spec), no binutils that would have previously successfully assembled our
rva22u64 output should fail due to the addition of 'B'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:RISC-V clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants