[RISCV] Gate unratified profiles behind -menable-experimental-extensions #92167
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-clang-driver Author: Alex Bradbury (asb) ChangesAs discussed in the last sync-up call, because these profiles are not yet finalised they shouldn't be exposed to users unless they opt-in to them (much like experimental extensions). We may later want to add a more specific flag, but reusing This is implemented using the new support for marking profiles s experimental added in #91993 to move the unratified profiles to RISCVExperimentalProfile and making the necessary changes to logic in RISCVISAInfo to handle this. This PR also includes a commit that adds baseline test coverage for profile strings in RISCVISAInfoTest's parseArch tests. Full diff: https://github.com/llvm/llvm-project/pull/92167.diff 5 Files Affected:
diff --git a/clang/test/Driver/riscv-profiles.c b/clang/test/Driver/riscv-profiles.c
index 298f301de3feb..55aa5b398cee9 100644
--- a/clang/test/Driver/riscv-profiles.c
+++ b/clang/test/Driver/riscv-profiles.c
@@ -111,7 +111,7 @@
// RVA22S64: "-target-feature" "+svinval"
// RVA22S64: "-target-feature" "+svpbmt"
-// RUN: %clang --target=riscv64 -### -c %s 2>&1 -march=rva23u64 \
+// RUN: %clang --target=riscv64 -### -c %s 2>&1 -march=rva23u64 -menable-experimental-extensions \
// RUN: | FileCheck -check-prefix=RVA23U64 %s
// RVA23U64: "-target-feature" "+m"
// RVA23U64: "-target-feature" "+a"
@@ -207,7 +207,7 @@
// RVA23S64: "-target-feature" "+svnapot"
// RVA23S64: "-target-feature" "+svpbmt"
-// RUN: %clang --target=riscv64 -### -c %s 2>&1 -march=rvb23u64 \
+// RUN: %clang --target=riscv64 -### -c %s 2>&1 -march=rvb23u64 -menable-experimental-extensions \
// RUN: | FileCheck -check-prefix=RVB23U64 %s
// RVB23U64: "-target-feature" "+m"
// RVB23U64: "-target-feature" "+a"
@@ -284,7 +284,7 @@
// RVB23S64: "-target-feature" "+svnapot"
// RVB23S64: "-target-feature" "+svpbmt"
-// RUN: %clang --target=riscv32 -### -c %s 2>&1 -march=rvm23u32 \
+// RUN: %clang --target=riscv32 -### -c %s 2>&1 -march=rvm23u32 -menable-experimental-extensions \
// RUN: | FileCheck -check-prefix=RVM23U32 %s
// RVM23U32: "-target-feature" "+m"
// RVM23U32: "-target-feature" "+zicbop"
@@ -322,3 +322,7 @@
// RUN: not %clang --target=riscv64 -### -c %s 2>&1 -march=rva22u64zfa | FileCheck -check-prefix=INVALID-ADDITIONAL %s
// INVALID-ADDITIONAL: error: invalid arch name 'rva22u64zfa', additional extensions must be after separator '_'
+
+// RUN: not %clang --target=riscv64 -### -c %s 2>&1 -march=rva23u64 | FileCheck -check-prefix=EXPERIMENTAL-NOFLAG %s
+// EXPERIMENTAL-NOFLAG: error: invalid arch name 'rva23u64'
+// EXPERIMENTAL-NOFLAG: requires '-menable-experimental-extensions' for profile 'rva23u64'
diff --git a/llvm/lib/Target/RISCV/RISCVProfiles.td b/llvm/lib/Target/RISCV/RISCVProfiles.td
index e56df33bd8cb0..c4a64681f5f12 100644
--- a/llvm/lib/Target/RISCV/RISCVProfiles.td
+++ b/llvm/lib/Target/RISCV/RISCVProfiles.td
@@ -13,6 +13,10 @@ class RISCVProfile<string name, list<SubtargetFeature> features>
// experimental.
bit Experimental = false;
}
+class RISCVExperimentalProfile<string name, list<SubtargetFeature> features>
+ : RISCVProfile<"experimental-"#name, features> {
+ let Experimental = true;
+}
defvar RVI20U32Features = [Feature32Bit, FeatureStdExtI];
defvar RVI20U64Features = [Feature64Bit, FeatureStdExtI];
@@ -201,8 +205,8 @@ def RVA20U64 : RISCVProfile<"rva20u64", RVA20U64Features>;
def RVA20S64 : RISCVProfile<"rva20s64", RVA20S64Features>;
def RVA22U64 : RISCVProfile<"rva22u64", RVA22U64Features>;
def RVA22S64 : RISCVProfile<"rva22s64", RVA22S64Features>;
-def RVA23U64 : RISCVProfile<"rva23u64", RVA23U64Features>;
-def RVA23S64 : RISCVProfile<"rva23s64", RVA23S64Features>;
-def RVB23U64 : RISCVProfile<"rvb23u64", RVB23U64Features>;
-def RVB23S64 : RISCVProfile<"rvb23s64", RVB23S64Features>;
-def RVM23U32 : RISCVProfile<"rvm23u32", RVM23U32Features>;
+def RVA23U64 : RISCVExperimentalProfile<"rva23u64", RVA23U64Features>;
+def RVA23S64 : RISCVExperimentalProfile<"rva23s64", RVA23S64Features>;
+def RVB23U64 : RISCVExperimentalProfile<"rvb23u64", RVB23U64Features>;
+def RVB23S64 : RISCVExperimentalProfile<"rvb23s64", RVB23S64Features>;
+def RVM23U32 : RISCVExperimentalProfile<"rvm23u32", RVM23U32Features>;
diff --git a/llvm/lib/TargetParser/RISCVISAInfo.cpp b/llvm/lib/TargetParser/RISCVISAInfo.cpp
index e22dd6032cb0c..b15d0f0f0be32 100644
--- a/llvm/lib/TargetParser/RISCVISAInfo.cpp
+++ b/llvm/lib/TargetParser/RISCVISAInfo.cpp
@@ -102,6 +102,10 @@ void llvm::riscvExtensionsHelp(StringMap<StringRef> DescMap) {
for (const auto &P : SupportedProfiles)
outs().indent(4) << P.Name << "\n";
+ outs() << "\nExperimental Profiles\n";
+ for (const auto &P : SupportedExperimentalProfiles)
+ outs().indent(4) << P.Name << "\n";
+
outs() << "\nUse -march to specify the target's extension.\n"
"For example, clang -march=rv32i_v1p0\n";
}
@@ -608,12 +612,25 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
XLen = 64;
} else {
// Try parsing as a profile.
- auto I = llvm::upper_bound(SupportedProfiles, Arch,
- [](StringRef Arch, const RISCVProfile &Profile) {
- return Arch < Profile.Name;
- });
-
- if (I != std::begin(SupportedProfiles) && Arch.starts_with((--I)->Name)) {
+ auto ProfileCmp = [](StringRef Arch, const RISCVProfile &Profile) {
+ return Arch < Profile.Name;
+ };
+ auto I = llvm::upper_bound(SupportedProfiles, Arch, ProfileCmp);
+ bool FoundProfile = I != std::begin(SupportedProfiles) &&
+ Arch.starts_with(std::prev(I)->Name);
+ if (!FoundProfile) {
+ I = llvm::upper_bound(SupportedExperimentalProfiles, Arch, ProfileCmp);
+ FoundProfile = (I != std::begin(SupportedExperimentalProfiles) &&
+ Arch.starts_with(std::prev(I)->Name));
+ if (FoundProfile && !EnableExperimentalExtension) {
+ return createStringError(errc::invalid_argument,
+ "requires '-menable-experimental-extensions' "
+ "for profile '" +
+ std::prev(I)->Name + "'");
+ }
+ }
+ if (FoundProfile) {
+ --I;
std::string NewArch = I->MArch.str();
StringRef ArchWithoutProfile = Arch.drop_front(I->Name.size());
if (!ArchWithoutProfile.empty()) {
diff --git a/llvm/test/CodeGen/RISCV/attributes.ll b/llvm/test/CodeGen/RISCV/attributes.ll
index 8f49f6648ad28..953ed5ee3795b 100644
--- a/llvm/test/CodeGen/RISCV/attributes.ll
+++ b/llvm/test/CodeGen/RISCV/attributes.ll
@@ -265,11 +265,11 @@
; RUN: llc -mtriple=riscv64 -mattr=+rva20s64 %s -o - | FileCheck --check-prefix=RVA20S64 %s
; RUN: llc -mtriple=riscv64 -mattr=+rva22u64 %s -o - | FileCheck --check-prefix=RVA22U64 %s
; 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=+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=+rvm23u32 %s -o - | FileCheck --check-prefix=RVM23U32 %s
+; RUN: llc -mtriple=riscv64 -mattr=+experimental-rva23u64 %s -o - | FileCheck --check-prefix=RVA23U64 %s
+; RUN: llc -mtriple=riscv64 -mattr=+experimental-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=riscv32 -mattr=+experimental-rvm23u32 %s -o - | FileCheck --check-prefix=RVM23U32 %s
; CHECK: .attribute 4, 16
diff --git a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
index 7f2d1eb8c0170..22fe31809319f 100644
--- a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
@@ -21,8 +21,8 @@ bool operator==(const RISCVISAUtils::ExtensionVersion &A,
}
TEST(ParseNormalizedArchString, RejectsInvalidChars) {
- for (StringRef Input :
- {"RV32", "rV64", "rv32i2P0", "rv64i2p0_A2p0", "rv32e2.0"}) {
+ for (StringRef Input : {"RV32", "rV64", "rv32i2P0", "rv64i2p0_A2p0",
+ "rv32e2.0", "rva20u64+zbc"}) {
EXPECT_EQ(
toString(RISCVISAInfo::parseNormalizedArchString(Input).takeError()),
"string may only contain [a-z0-9_]");
@@ -667,6 +667,72 @@ TEST(ParseArchString, RejectsConflictingExtensions) {
}
}
+TEST(ParseArchString, RejectsUnrecognizedProfileNames) {
+ for (StringRef Input : {"rvi23u99", "rvz23u64", "rva99u32"}) {
+ EXPECT_EQ(toString(RISCVISAInfo::parseArchString(Input, true).takeError()),
+ "string must begin with rv32{i,e,g}, rv64{i,e,g}, or a supported "
+ "profile name");
+ }
+}
+
+TEST(ParseArchString, RejectsProfilesWithUnseparatedExtraExtensions) {
+ for (StringRef Input : {"rvi20u32m", "rvi20u64c"}) {
+ EXPECT_EQ(toString(RISCVISAInfo::parseArchString(Input, true).takeError()),
+ "additional extensions must be after separator '_'");
+ }
+}
+
+TEST(ParseArchString, AcceptsBareProfileNames) {
+ auto MaybeRVA20U64 = RISCVISAInfo::parseArchString("rva20u64", true);
+ ASSERT_THAT_EXPECTED(MaybeRVA20U64, Succeeded());
+ const auto &Exts = (*MaybeRVA20U64)->getExtensions();
+ EXPECT_EQ(Exts.size(), 13UL);
+ EXPECT_EQ(Exts.count("i"), 1U);
+ EXPECT_EQ(Exts.count("m"), 1U);
+ EXPECT_EQ(Exts.count("f"), 1U);
+ EXPECT_EQ(Exts.count("a"), 1U);
+ EXPECT_EQ(Exts.count("d"), 1U);
+ EXPECT_EQ(Exts.count("c"), 1U);
+ EXPECT_EQ(Exts.count("za128rs"), 1U);
+ EXPECT_EQ(Exts.count("zicntr"), 1U);
+ EXPECT_EQ(Exts.count("ziccif"), 1U);
+ EXPECT_EQ(Exts.count("zicsr"), 1U);
+ EXPECT_EQ(Exts.count("ziccrse"), 1U);
+ EXPECT_EQ(Exts.count("ziccamoa"), 1U);
+ EXPECT_EQ(Exts.count("zicclsm"), 1U);
+
+ auto MaybeRVA23U64 = RISCVISAInfo::parseArchString("rva23u64", true);
+ ASSERT_THAT_EXPECTED(MaybeRVA23U64, Succeeded());
+ EXPECT_GT((*MaybeRVA23U64)->getExtensions().size(), 13UL);
+}
+
+TEST(ParseArchSTring, AcceptsProfileNamesWithSeparatedAdditionalExtensions) {
+ auto MaybeRVI20U64 = RISCVISAInfo::parseArchString("rvi20u64_m_zba", true);
+ ASSERT_THAT_EXPECTED(MaybeRVI20U64, Succeeded());
+ const auto &Exts = (*MaybeRVI20U64)->getExtensions();
+ EXPECT_EQ(Exts.size(), 3UL);
+ EXPECT_EQ(Exts.count("i"), 1U);
+ EXPECT_EQ(Exts.count("m"), 1U);
+ EXPECT_EQ(Exts.count("zba"), 1U);
+}
+
+TEST(ParseArchString,
+ RejectsProfilesWithAdditionalExtensionsGivenAlreadyInProfile) {
+ // This test was added to document the current behaviour. Discussion isn't
+ // believed to have taken place about if this is desirable or not.
+ EXPECT_EQ(
+ toString(
+ RISCVISAInfo::parseArchString("rva20u64_zicntr", true).takeError()),
+ "duplicated standard user-level extension 'zicntr'");
+}
+
+TEST(ParseArchString,
+ RejectsExperimentalProfilesIfEnableExperimentalExtensionsNotSet) {
+ EXPECT_EQ(
+ toString(RISCVISAInfo::parseArchString("rva23u64", false).takeError()),
+ "requires '-menable-experimental-extensions' for profile 'rva23u64'");
+}
+
TEST(ToFeatures, IIsDroppedAndExperimentalExtensionsArePrefixed) {
auto MaybeISAInfo1 =
RISCVISAInfo::parseArchString("rv64im_ztso", true, false);
@@ -1014,12 +1080,14 @@ Supported Profiles
rva20u64
rva22s64
rva22u64
+ rvi20u32
+ rvi20u64
+
+Experimental Profiles
rva23s64
rva23u64
rvb23s64
rvb23u64
- rvi20u32
- rvi20u64
rvm23u32
Use -march to specify the target's extension.
|
As discussed in the last sync-up call, because these profiles are not yet finalised they shouldn't be exposed to users unless they opt-in to them (much like experimental extensions). We may later want to add a more specific flag, but reusing `-menable-experimental-extensions` solves the immediate problem.
asb
force-pushed
the
2024q2-riscv-start-using-experimental-profiles
branch
from
6c97493
to
5159fce
Compare
|
Thank you for the reviews - I've gone ahead and merged. One thing from the added tests that's worth highlighting perhaps is |
mub-at-arm
pushed a commit
to mub-at-arm/llvm-project
that referenced
this pull request
May 16, 2024
…ons (llvm#92167) As discussed in the last sync-up call, because these profiles are not yet finalised they shouldn't be exposed to users unless they opt-in to them (much like experimental extensions). We may later want to add a more specific flag, but reusing `-menable-experimental-extensions` solves the immediate problem. This is implemented using the new support for marking profiles s experimental added in llvm#91993 to move the unratified profiles to RISCVExperimentalProfile and making the necessary changes to logic in RISCVISAInfo to handle this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in the last sync-up call, because these profiles are not yet finalised they shouldn't be exposed to users unless they opt-in to them (much like experimental extensions). We may later want to add a more specific flag, but reusing
-menable-experimental-extensionssolves the immediate problem.This is implemented using the new support for marking profiles s experimental added in #91993 to move the unratified profiles to RISCVExperimentalProfile and making the necessary changes to logic in RISCVISAInfo to handle this.
This PR also includes a commit that adds baseline test coverage for profile strings in RISCVISAInfoTest's parseArch tests.