-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[RISCV] Add B extension #76893
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] Add B extension #76893
Conversation
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-clang Author: Wang Pengcheng (wangpc-pp) ChangesIt seems that we have According to the spec, Though it hasn't been ratified, I set its version to Full diff: https://github.com/llvm/llvm-project/pull/76893.diff 5 Files Affected:
diff --git a/clang/test/Preprocessor/riscv-target-features.c b/clang/test/Preprocessor/riscv-target-features.c
index 02d8d34116f804..783edfe7301a5f 100644
--- a/clang/test/Preprocessor/riscv-target-features.c
+++ b/clang/test/Preprocessor/riscv-target-features.c
@@ -5,6 +5,7 @@
// CHECK-NOT: __riscv_a {{.*$}}
// CHECK-NOT: __riscv_atomic
+// CHECK-NOT: __riscv_b {{.*$}}
// CHECK-NOT: __riscv_c {{.*$}}
// CHECK-NOT: __riscv_compressed {{.*$}}
// CHECK-NOT: __riscv_d {{.*$}}
@@ -150,6 +151,17 @@
// CHECK-A-EXT: __riscv_a 2001000{{$}}
// CHECK-A-EXT: __riscv_atomic 1
+// RUN: %clang --target=riscv32-unknown-linux-gnu \
+// RUN: -march=rv32ib -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-B-EXT %s
+// RUN: %clang --target=riscv64-unknown-linux-gnu \
+// RUN: -march=rv64ib -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-B-EXT %s
+// CHECK-B-EXT: __riscv_b 1000000{{$}}
+// CHECK-B-EXT: __riscv_zba 1000000{{$}}
+// CHECK-B-EXT: __riscv_zbb 1000000{{$}}
+// CHECK-B-EXT: __riscv_zbs 1000000{{$}}
+
// RUN: %clang --target=riscv32-unknown-linux-gnu \
// RUN: -march=rv32ic -x c -E -dM %s \
// RUN: -o - | FileCheck --check-prefix=CHECK-C-EXT %s
diff --git a/llvm/docs/RISCVUsage.rst b/llvm/docs/RISCVUsage.rst
index 99c7146825f5ee..05634702595018 100644
--- a/llvm/docs/RISCVUsage.rst
+++ b/llvm/docs/RISCVUsage.rst
@@ -85,6 +85,7 @@ on support follow.
Extension Status
=============== =========================================================
``A`` Supported
+ ``B`` Supported
``C`` Supported
``D`` Supported
``F`` Supported
diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index a9b7e209915a13..f9c5bd2eb2bdbb 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -51,6 +51,7 @@ static const char *RISCVGImplications[] = {
// NOTE: This table should be sorted alphabetically by extension name.
static const RISCVSupportedExtension SupportedExtensions[] = {
{"a", RISCVExtensionVersion{2, 1}},
+ {"b", RISCVExtensionVersion{1, 0}},
{"c", RISCVExtensionVersion{2, 0}},
{"d", RISCVExtensionVersion{2, 2}},
{"e", RISCVExtensionVersion{2, 0}},
@@ -997,6 +998,7 @@ Error RISCVISAInfo::checkDependency() {
return Error::success();
}
+static const char *ImpliedExtsB[] = {"zba", "zbb", "zbs"};
static const char *ImpliedExtsD[] = {"f"};
static const char *ImpliedExtsF[] = {"zicsr"};
static const char *ImpliedExtsV[] = {"zvl128b", "zve64d"};
@@ -1071,6 +1073,7 @@ struct ImpliedExtsEntry {
// Note: The table needs to be sorted by name.
static constexpr ImpliedExtsEntry ImpliedExts[] = {
+ {{"b"}, {ImpliedExtsB}},
{{"d"}, {ImpliedExtsD}},
{{"f"}, {ImpliedExtsF}},
{{"v"}, {ImpliedExtsV}},
diff --git a/llvm/lib/Target/RISCV/RISCVFeatures.td b/llvm/lib/Target/RISCV/RISCVFeatures.td
index 59b202606dadaf..0201f4837733a0 100644
--- a/llvm/lib/Target/RISCV/RISCVFeatures.td
+++ b/llvm/lib/Target/RISCV/RISCVFeatures.td
@@ -202,6 +202,14 @@ def HasStdExtZbs : Predicate<"Subtarget->hasStdExtZbs()">,
AssemblerPredicate<(all_of FeatureStdExtZbs),
"'Zbs' (Single-Bit Instructions)">;
+def FeatureStdExtB
+ : SubtargetFeature<"b", "HasStdExtB", "true",
+ "'B' (the collection of the Zba, Zbb, Zbs extensions)",
+ [FeatureStdExtZba, FeatureStdExtZbb, FeatureStdExtZbs]>;
+def HasStdExtB : Predicate<"Subtarget->hasStdExtB()">,
+ AssemblerPredicate<(all_of FeatureStdExtB),
+ "'B' (the collection of the Zba, Zbb, Zbs extensions)">;
+
def FeatureStdExtZbkb
: SubtargetFeature<"zbkb", "HasStdExtZbkb", "true",
"'Zbkb' (Bitmanip instructions for Cryptography)">;
diff --git a/llvm/test/CodeGen/RISCV/attributes.ll b/llvm/test/CodeGen/RISCV/attributes.ll
index 9a6e78c09ad8c3..522e2c8a30e1e6 100644
--- a/llvm/test/CodeGen/RISCV/attributes.ll
+++ b/llvm/test/CodeGen/RISCV/attributes.ll
@@ -5,6 +5,7 @@
; RUN: llc -mtriple=riscv32 -mattr=+zmmul %s -o - | FileCheck --check-prefixes=CHECK,RV32ZMMUL %s
; RUN: llc -mtriple=riscv32 -mattr=+m,+zmmul %s -o - | FileCheck --check-prefixes=CHECK,RV32MZMMUL %s
; RUN: llc -mtriple=riscv32 -mattr=+a %s -o - | FileCheck --check-prefixes=CHECK,RV32A %s
+; RUN: llc -mtriple=riscv32 -mattr=+b %s -o - | FileCheck --check-prefixes=CHECK,RV32B %s
; RUN: llc -mtriple=riscv32 -mattr=+f %s -o - | FileCheck --check-prefixes=CHECK,RV32F %s
; RUN: llc -mtriple=riscv32 -mattr=+d %s -o - | FileCheck --check-prefixes=CHECK,RV32D %s
; RUN: llc -mtriple=riscv32 -mattr=+c %s -o - | FileCheck --check-prefixes=CHECK,RV32C %s
@@ -100,6 +101,7 @@
; RUN: llc -mtriple=riscv64 -mattr=+zmmul %s -o - | FileCheck --check-prefixes=CHECK,RV64ZMMUL %s
; RUN: llc -mtriple=riscv64 -mattr=+m,+zmmul %s -o - | FileCheck --check-prefixes=CHECK,RV64MZMMUL %s
; RUN: llc -mtriple=riscv64 -mattr=+a %s -o - | FileCheck --check-prefixes=CHECK,RV64A %s
+; RUN: llc -mtriple=riscv64 -mattr=+b %s -o - | FileCheck --check-prefixes=CHECK,RV64B %s
; RUN: llc -mtriple=riscv64 -mattr=+f %s -o - | FileCheck --check-prefixes=CHECK,RV64F %s
; RUN: llc -mtriple=riscv64 -mattr=+d %s -o - | FileCheck --check-prefixes=CHECK,RV64D %s
; RUN: llc -mtriple=riscv64 -mattr=+c %s -o - | FileCheck --check-prefixes=CHECK,RV64C %s
@@ -195,6 +197,7 @@
; RV32ZMMUL: .attribute 5, "rv32i2p1_zmmul1p0"
; RV32MZMMUL: .attribute 5, "rv32i2p1_m2p0_zmmul1p0"
; RV32A: .attribute 5, "rv32i2p1_a2p1"
+; RV32B: .attribute 5, "rv32i2p1_b1p0_zba1p0_zbb1p0_zbs1p0"
; RV32F: .attribute 5, "rv32i2p1_f2p2_zicsr2p0"
; RV32D: .attribute 5, "rv32i2p1_f2p2_d2p2_zicsr2p0"
; RV32C: .attribute 5, "rv32i2p1_c2p0"
@@ -289,6 +292,7 @@
; RV64ZMMUL: .attribute 5, "rv64i2p1_zmmul1p0"
; RV64MZMMUL: .attribute 5, "rv64i2p1_m2p0_zmmul1p0"
; RV64A: .attribute 5, "rv64i2p1_a2p1"
+; RV64B: .attribute 5, "rv64i2p1_b1p0_zba1p0_zbb1p0_zbs1p0"
; RV64F: .attribute 5, "rv64i2p1_f2p2_zicsr2p0"
; RV64D: .attribute 5, "rv64i2p1_f2p2_d2p2_zicsr2p0"
; RV64C: .attribute 5, "rv64i2p1_c2p0"
|
I would suggest set it as 0.1 rather than 1.0, and I gonna to ask Ved to add version info as well... |
And forgot to say: thanks for raise this issue! I didn't aware b extension is back again until this PR created. |
Then also needs to move behind -menable-experimental-extensions. |
Issue created at riscv-b: riscv/riscv-b#3 |
Yes, this is why I set version to 1.0. |
Thanks! If someone sets zba_zbb_zbs, should b be inferred? This patch needs an LLVM release note as well, and agreed we should await on a versioning decision. |
Yes, but it will break |
It tagged with 0.1 now :) |
I will hold this PR util B extension is ratified (I think it won't take too long). Or, at least after llvm 18 branch is created. |
52ed8e0
to
c4f7441
Compare
It passed public review[1] and merged into riscv-isa-manual[2], so I think it's time to mark it as 1.0 and moving forward :) [1] https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/KetVUCQkfK4/m/Y3Dbd2pvAAAJ?utm_medium=email&utm_source=footer |
Could you add |
That's probably true - though historically I've been using https://wiki.riscv.org/display/HOME/Recently+Ratified+Extensions to determine if a spec is ratified or not, now moved to https://wiki.riscv.org/display/HOME/Ratified+Extensions seemingly. B doesn't seem to be listed yet. @jjscheel - is it just not added to the wiki page yet, or is it technically not yet ratified? (I know it's really a trivial "extension" so might be a special case) |
Jeff told me it's still need wait TSC vote for the ratification, anyway it will ratify this month. |
Ping. The B extension has been ratified and I have rebased this PR. |
@@ -920,8 +920,8 @@ void RISCVISAInfo::updateImplication() { | |||
} | |||
|
|||
static constexpr StringLiteral CombineIntoExts[] = { | |||
{"zk"}, {"zkn"}, {"zks"}, {"zvkn"}, {"zvknc"}, | |||
{"zvkng"}, {"zvks"}, {"zvksc"}, {"zvksg"}, | |||
{"b"}, {"zk"}, {"zkn"}, {"zks"}, {"zvkn"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned this will make us not interoperate with older versions of binutils that don't know about B.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kito-cheng WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this part.
We can support combination of B extension in a separate patch if we need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this from CombineIntoExts
may cause __riscv_b
become less useful I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this from
CombineIntoExts
may cause__riscv_b
become less useful I think?
Yeah, but I think @topperc's concern makes sense as well. Will binutils complain or just ignore unknown extensions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
It seems that we have `B` extension again: https://github.com/riscv/riscv-b According to the spec, `B` extension represents the collection of the `Zba`, `Zbb`, `Zbs` extensions.
It seems that we have `B` extension again: https://github.com/riscv/riscv-b According to the spec, `B` extension represents the collection of the `Zba`, `Zbb`, `Zbs` extensions. Though it hasn't been ratified, I set its version to `1.0`.
This was introduced in the now-ratified RVA23 profile (and also added to the RVA22 text) as a simple way of referring to H plus the set of supervisor extensions required by RVA23. https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc This patch simply defines the extension. The next patch will adjust the RVA23 profile to use it, and at that point I think we will be ready to mark RVA23 as non-experimental. Note that I haven't made it so if you enable all extensions that constitute Sha, Sha is implied. Per #76893 (adding 'B'), the concern is making this implication might break older external assemblers. Perhaps this is less of a concern given the relative frequency of `-march=${foo}_zba_zbb_zbs` vs the collection of H extensions. If we did want to add that implication, we'd probably want to add it in a separate patch so it can be easily reverted if found to cause problems.
This was introduced in the now-ratified RVA23 profile (and also added to the RVA22 text) as a simple way of referring to H plus the set of supervisor extensions required by RVA23. https://github.com/riscv/riscv-profiles/blob/main/src/rva23-profile.adoc This patch simply defines the extension. The next patch will adjust the RVA23 profile to use it, and at that point I think we will be ready to mark RVA23 as non-experimental. Note that I haven't made it so if you enable all extensions that constitute Sha, Sha is implied. Per llvm#76893 (adding 'B'), the concern is making this implication might break older external assemblers. Perhaps this is less of a concern given the relative frequency of `-march=${foo}_zba_zbb_zbs` vs the collection of H extensions. If we did want to add that implication, we'd probably want to add it in a separate patch so it can be easily reverted if found to cause problems.
It seems that we have
B
extension again: https://github.com/riscv/riscv-bAccording to the spec,
B
extension represents the collection ofthe
Zba
,Zbb
,Zbs
extensions.Though it hasn't been ratified, I set its version to
1.0
.