[RISCV] Relax march string order constraint #78120
Merged
Conversation
This file contains hidden or 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-llvm-support Author: Piyou Chen (BeMg) ChangesAddress the riscv-non-isa/riscv-toolchain-conventions#14 This patch relax the
Patch is 20.84 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/78120.diff 3 Files Affected:
diff --git a/clang/test/Driver/riscv-arch.c b/clang/test/Driver/riscv-arch.c
index 0ac81ea982f1b6..38de95e4fbf7aa 100644
--- a/clang/test/Driver/riscv-arch.c
+++ b/clang/test/Driver/riscv-arch.c
@@ -156,9 +156,8 @@
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32L %s
// RV32L: error: invalid arch name 'rv32l'
-// RUN: not %clang --target=riscv32-unknown-elf -march=rv32imadf -### %s \
-// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32IMADF %s
-// RV32IMADF: error: invalid arch name 'rv32imadf'
+// RUN: %clang --target=riscv32-unknown-elf -march=rv32imadf -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck %s
// RUN: not %clang --target=riscv32-unknown-elf -march=rv32imm -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32IMM %s
@@ -184,9 +183,8 @@
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64L %s
// RV64L: error: invalid arch name 'rv64l'
-// RUN: not %clang --target=riscv64-unknown-elf -march=rv64imadf -### %s \
-// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64IMADF %s
-// RV64IMADF: error: invalid arch name 'rv64imadf'
+// RUN: %clang --target=riscv64-unknown-elf -march=rv64imadf -### %s \
+// RUN: -fsyntax-only 2>&1 | FileCheck %s
// RUN: not %clang --target=riscv64-unknown-elf -march=rv64imm -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64IMM %s
@@ -216,7 +214,7 @@
// RUN: not %clang --target=riscv32-unknown-elf -march=rv32imcq -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-ORDER %s
// RV32-ORDER: error: invalid arch name 'rv32imcq',
-// RV32-ORDER: standard user-level extension not given in canonical order 'q'
+// RV32-ORDER: unsupported standard user-level extension 'q'
// RUN: not %clang --target=riscv32-unknown-elf -march=rv32izvl64b -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-ZVL64B-ER %s
@@ -318,7 +316,7 @@
// RUN: not %clang --target=riscv32-unknown-elf -march=rv32ixabc_a -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-PREFIX %s
// RV32-PREFIX: error: invalid arch name 'rv32ixabc_a',
-// RV32-PREFIX: invalid extension prefix 'a'
+// RV32-PREFIX: unsupported non-standard user-level extension 'xabc'
// RUN: not %clang --target=riscv32-unknown-elf -march=rv32ixdef_sabc -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-X-ORDER %s
diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index 390d950486a795..865ca48aeb90d1 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -695,6 +695,106 @@ RISCVISAInfo::parseNormalizedArchString(StringRef Arch) {
return std::move(ISAInfo);
}
+static Error splitExtsByUnderscore(StringRef Exts,
+ std::vector<std::string> &SplitedExts) {
+ SmallVector<StringRef, 8> Split;
+ if (Exts.empty())
+ return Error::success();
+
+ Exts.split(Split, "_");
+
+ for (auto Ext : Split) {
+ if (Ext.empty())
+ return createStringError(errc::invalid_argument,
+ "extension name missing after separator '_'");
+
+ SplitedExts.push_back(Ext.str());
+ }
+ return Error::success();
+}
+
+static Error processMultiLetterExtension(
+ StringRef RawExt, SmallVector<std::string, 8> &SeenExts,
+ SmallVector<RISCVISAInfo::ExtensionVersion, 8> &ExtsVersion,
+ bool IgnoreUnknown, bool EnableExperimentalExtension,
+ bool ExperimentalExtensionVersionCheck) {
+ StringRef Type = getExtensionType(RawExt);
+ StringRef Desc = getExtensionTypeDesc(RawExt);
+ auto Pos = findLastNonVersionCharacter(RawExt) + 1;
+ StringRef Name(RawExt.substr(0, Pos));
+ StringRef Vers(RawExt.substr(Pos));
+
+ if (Type.empty()) {
+ if (IgnoreUnknown)
+ return Error::success();
+ return createStringError(errc::invalid_argument,
+ "invalid extension prefix '" + RawExt + "'");
+ }
+
+ if (!IgnoreUnknown && Name.size() == Type.size())
+ return createStringError(errc::invalid_argument,
+ "%s name missing after '%s'", Desc.str().c_str(),
+ Type.str().c_str());
+
+ unsigned Major, Minor, ConsumeLength;
+ if (auto E = getExtensionVersion(Name, Vers, Major, Minor, ConsumeLength,
+ EnableExperimentalExtension,
+ ExperimentalExtensionVersionCheck)) {
+ if (IgnoreUnknown) {
+ consumeError(std::move(E));
+ return Error::success();
+ }
+ return E;
+ }
+
+ // Check if duplicated extension.
+ if (!IgnoreUnknown && llvm::is_contained(SeenExts, Name))
+ return createStringError(errc::invalid_argument, "duplicated %s '%s'",
+ Desc.str().c_str(), Name.str().c_str());
+
+ if (IgnoreUnknown && !RISCVISAInfo::isSupportedExtension(Name))
+ return Error::success();
+
+ SeenExts.push_back(Name.str());
+ ExtsVersion.push_back({Major, Minor});
+ return Error::success();
+}
+
+static Error processSingleLetterExtension(
+ StringRef &RawExt, SmallVector<std::string, 8> &SeenExts,
+ SmallVector<RISCVISAInfo::ExtensionVersion, 8> &ExtsVersion,
+ bool IgnoreUnknown, bool EnableExperimentalExtension,
+ bool ExperimentalExtensionVersionCheck) {
+ unsigned Major, Minor, ConsumeLength;
+ StringRef Name = RawExt.take_front(1);
+ RawExt.consume_front(Name);
+ if (auto E = getExtensionVersion(Name, RawExt, Major, Minor, ConsumeLength,
+ EnableExperimentalExtension,
+ ExperimentalExtensionVersionCheck)) {
+ if (IgnoreUnknown) {
+ consumeError(std::move(E));
+ RawExt = RawExt.substr(ConsumeLength);
+ return Error::success();
+ }
+ return E;
+ }
+
+ RawExt = RawExt.substr(ConsumeLength);
+
+ // Check if duplicated extension.
+ if (!IgnoreUnknown && llvm::is_contained(SeenExts, Name))
+ return createStringError(errc::invalid_argument,
+ "duplicated standard user-level extension '%s'",
+ Name.str().c_str());
+
+ if (IgnoreUnknown && !RISCVISAInfo::isSupportedExtension(Name))
+ return Error::success();
+
+ SeenExts.push_back(Name.str());
+ ExtsVersion.push_back({Major, Minor});
+ return Error::success();
+}
+
llvm::Expected<std::unique_ptr<RISCVISAInfo>>
RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
bool ExperimentalExtensionVersionCheck,
@@ -715,6 +815,8 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
unsigned XLen = HasRV64 ? 64 : 32;
std::unique_ptr<RISCVISAInfo> ISAInfo(new RISCVISAInfo(XLen));
+ SmallVector<std::string, 8> SeenExts;
+ SmallVector<RISCVISAInfo::ExtensionVersion, 8> ExtsVersion;
// The canonical order specified in ISA manual.
// Ref: Table 22.1 in RISC-V User-Level ISA V2.2
@@ -745,17 +847,6 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
// Skip rvxxx
StringRef Exts = Arch.substr(5);
- // Remove multi-letter standard extensions, non-standard extensions and
- // supervisor-level extensions. They have 'z', 'x', 's' prefixes.
- // Parse them at the end.
- // Find the very first occurrence of 's', 'x' or 'z'.
- StringRef OtherExts;
- size_t Pos = Exts.find_first_of("zsx");
- if (Pos != StringRef::npos) {
- OtherExts = Exts.substr(Pos);
- Exts = Exts.substr(0, Pos);
- }
-
unsigned Major, Minor, ConsumeLength;
if (Baseline == 'g') {
// Versions for g are disallowed, and this was checked for previously.
@@ -765,9 +856,11 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
// version since the we don't have clear version scheme for that on
// ISA spec.
for (const auto *Ext : RISCVGImplications) {
- if (auto Version = findDefaultVersion(Ext))
- ISAInfo->addExtension(Ext, *Version);
- else
+ if (auto Version = findDefaultVersion(Ext)) {
+ // Postpone AddExtension until end of this function
+ SeenExts.push_back(Ext);
+ ExtsVersion.push_back({Version->Major, Version->Minor});
+ } else
llvm_unreachable("Default extension version not found?");
}
} else {
@@ -785,7 +878,9 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
Minor = Version->Minor;
}
- ISAInfo->addExtension(StringRef(&Baseline, 1), {Major, Minor});
+ // Postpone AddExtension until end of this function
+ SeenExts.push_back(StringRef(&Baseline, 1).str());
+ ExtsVersion.push_back({Major, Minor});
}
// Consume the base ISA version number and any '_' between rvxxx and the
@@ -793,145 +888,51 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
Exts = Exts.drop_front(ConsumeLength);
Exts.consume_front("_");
- auto StdExtsItr = StdExts.begin();
- auto StdExtsEnd = StdExts.end();
- auto GoToNextExt = [](StringRef::iterator &I, unsigned ConsumeLength,
- StringRef::iterator E) {
- I += 1 + ConsumeLength;
- if (I != E && *I == '_')
- ++I;
- };
- for (auto I = Exts.begin(), E = Exts.end(); I != E;) {
- char C = *I;
-
- // Check ISA extensions are specified in the canonical order.
- while (StdExtsItr != StdExtsEnd && *StdExtsItr != C)
- ++StdExtsItr;
-
- if (StdExtsItr == StdExtsEnd) {
- // Either c contains a valid extension but it was not given in
- // canonical order or it is an invalid extension.
- if (StdExts.contains(C)) {
- return createStringError(
- errc::invalid_argument,
- "standard user-level extension not given in canonical order '%c'",
- C);
- }
-
- return createStringError(errc::invalid_argument,
- "invalid standard user-level extension '%c'", C);
- }
-
- // Move to next char to prevent repeated letter.
- ++StdExtsItr;
-
- StringRef Next;
- unsigned Major, Minor, ConsumeLength;
- if (std::next(I) != E)
- Next = StringRef(std::next(I), E - std::next(I));
- if (auto E = getExtensionVersion(StringRef(&C, 1), Next, Major, Minor,
- ConsumeLength, EnableExperimentalExtension,
- ExperimentalExtensionVersionCheck)) {
- if (IgnoreUnknown) {
- consumeError(std::move(E));
- GoToNextExt(I, ConsumeLength, Exts.end());
- continue;
- }
- return std::move(E);
- }
-
- // The order is OK, then push it into features.
- // Currently LLVM supports only "mafdcvh".
- if (!isSupportedExtension(StringRef(&C, 1))) {
- if (IgnoreUnknown) {
- GoToNextExt(I, ConsumeLength, Exts.end());
- continue;
- }
- return createStringError(errc::invalid_argument,
- "unsupported standard user-level extension '%c'",
- C);
- }
- ISAInfo->addExtension(StringRef(&C, 1), {Major, Minor});
-
- // Consume full extension name and version, including any optional '_'
- // between this extension and the next
- GoToNextExt(I, ConsumeLength, Exts.end());
- }
-
- // Handle other types of extensions other than the standard
- // general purpose and standard user-level extensions.
- // Parse the ISA string containing non-standard user-level
- // extensions, standard supervisor-level extensions and
- // non-standard supervisor-level extensions.
- // These extensions start with 'z', 's', 'x' prefixes, might have a version
- // number (major, minor) and are separated by a single underscore '_'. We do
- // not enforce a canonical order for them.
- // Set the hardware features for the extensions that are supported.
-
- // Multi-letter extensions are seperated by a single underscore
- // as described in RISC-V User-Level ISA V2.2.
- SmallVector<StringRef, 8> Split;
- OtherExts.split(Split, '_');
-
- SmallVector<StringRef, 8> AllExts;
- if (Split.size() > 1 || Split[0] != "") {
- for (StringRef Ext : Split) {
- if (Ext.empty())
- return createStringError(errc::invalid_argument,
- "extension name missing after separator '_'");
-
- StringRef Type = getExtensionType(Ext);
- StringRef Desc = getExtensionTypeDesc(Ext);
- auto Pos = findLastNonVersionCharacter(Ext) + 1;
- StringRef Name(Ext.substr(0, Pos));
- StringRef Vers(Ext.substr(Pos));
-
- if (Type.empty()) {
- if (IgnoreUnknown)
- continue;
- return createStringError(errc::invalid_argument,
- "invalid extension prefix '" + Ext + "'");
- }
-
- if (!IgnoreUnknown && Name.size() == Type.size()) {
+ std::vector<std::string> SplitedExts;
+ if (auto E = splitExtsByUnderscore(Exts, SplitedExts))
+ return std::move(E);
+
+ for (auto Ext : SplitedExts) {
+ StringRef CurrExt = Ext;
+ while (!CurrExt.empty()) {
+ if (AllStdExts.contains(CurrExt.front())) {
+ if (auto E = processSingleLetterExtension(
+ CurrExt, SeenExts, ExtsVersion, IgnoreUnknown,
+ EnableExperimentalExtension, ExperimentalExtensionVersionCheck))
+ return E;
+ } else if (CurrExt.front() == 'z' || CurrExt.front() == 's' ||
+ CurrExt.front() == 'x') {
+ if (auto E = processMultiLetterExtension(
+ CurrExt, SeenExts, ExtsVersion, IgnoreUnknown,
+ EnableExperimentalExtension, ExperimentalExtensionVersionCheck))
+ return E;
+ // Multi-letter extension must be seperate following extension with
+ // underscore
+ break;
+ } else {
+ // FIXME: Could it be ignored by IgnoreUnknown?
return createStringError(errc::invalid_argument,
- "%s name missing after '%s'",
- Desc.str().c_str(), Type.str().c_str());
- }
-
- unsigned Major, Minor, ConsumeLength;
- if (auto E = getExtensionVersion(Name, Vers, Major, Minor, ConsumeLength,
- EnableExperimentalExtension,
- ExperimentalExtensionVersionCheck)) {
- if (IgnoreUnknown) {
- consumeError(std::move(E));
- continue;
- }
- return std::move(E);
- }
-
- // Check if duplicated extension.
- if (!IgnoreUnknown && llvm::is_contained(AllExts, Name)) {
- return createStringError(errc::invalid_argument, "duplicated %s '%s'",
- Desc.str().c_str(), Name.str().c_str());
+ "invalid standard user-level extension '%c'",
+ CurrExt.front());
}
-
- if (IgnoreUnknown && !isSupportedExtension(Name))
- continue;
-
- ISAInfo->addExtension(Name, {Major, Minor});
- // Extension format is correct, keep parsing the extensions.
- // TODO: Save Type, Name, Major, Minor to avoid parsing them later.
- AllExts.push_back(Name);
}
}
- for (auto Ext : AllExts) {
- if (!isSupportedExtension(Ext)) {
- StringRef Desc = getExtensionTypeDesc(getExtensionType(Ext));
- return createStringError(errc::invalid_argument, "unsupported %s '%s'",
- Desc.str().c_str(), Ext.str().c_str());
+ // Check all Extensions are supported.
+ for (size_t Idx = 0; Idx < SeenExts.size(); Idx++) {
+ if (!RISCVISAInfo::isSupportedExtension(SeenExts[Idx])) {
+ if (SeenExts[Idx].size() == 1) {
+ return createStringError(
+ errc::invalid_argument,
+ "unsupported standard user-level extension '%s'",
+ SeenExts[Idx].c_str());
+ }
+ return createStringError(
+ errc::invalid_argument, "unsupported %s '%s'",
+ getExtensionTypeDesc(SeenExts[Idx]).str().c_str(),
+ SeenExts[Idx].c_str());
}
+ ISAInfo->addExtension(SeenExts[Idx], ExtsVersion[Idx]);
}
return RISCVISAInfo::postProcessAndChecking(std::move(ISAInfo));
diff --git a/llvm/unittests/Support/RISCVISAInfoTest.cpp b/llvm/unittests/Support/RISCVISAInfoTest.cpp
index 997551e5c44c09..41d40f143eb391 100644
--- a/llvm/unittests/Support/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/Support/RISCVISAInfoTest.cpp
@@ -203,24 +203,6 @@ TEST(ParseArchString, AcceptsSupportedBaseISAsAndSetsXLenAndFLen) {
EXPECT_EQ(InfoRV64G.getFLen(), 64U);
}
-TEST(ParseArchString, RequiresCanonicalOrderForSingleLetterExtensions) {
- EXPECT_EQ(
- toString(RISCVISAInfo::parseArchString("rv64idf", true).takeError()),
- "standard user-level extension not given in canonical order 'f'");
- EXPECT_EQ(
- toString(RISCVISAInfo::parseArchString("rv32iam", true).takeError()),
- "standard user-level extension not given in canonical order 'm'");
- EXPECT_EQ(
- toString(
- RISCVISAInfo::parseArchString("rv32i_zfinx_a", true).takeError()),
- "invalid extension prefix 'a'");
- // Canonical ordering not required for z*, s*, and x* extensions.
- EXPECT_THAT_EXPECTED(
- RISCVISAInfo::parseArchString(
- "rv64imafdc_xsfvcp_zicsr_xtheadba_svnapot_zawrs", true),
- Succeeded());
-}
-
TEST(ParseArchString, RejectsUnrecognizedExtensionNamesByDefault) {
EXPECT_EQ(toString(RISCVISAInfo::parseArchString("rv64ib", true).takeError()),
"unsupported standard user-level extension 'b'");
@@ -337,10 +319,79 @@ TEST(ParseArchString, AcceptsUnderscoreSplittingExtensions) {
}
}
+TEST(ParseArchString, AcceptsRelaxSingleLetterExtensions) {
+ for (StringRef Input :
+ {"rv32imfad", "rv32im_fa_d", "rv32im2p0fad", "rv32i2p1m2p0fad"}) {
+ auto MaybeISAInfo = RISCVISAInfo::parseArchString(Input, true);
+ ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
+ RISCVISAInfo::OrderedExtensionMap Exts = (*MaybeISAInfo)->getExtensions();
+ EXPECT_EQ(Exts.size(), 6UL);
+ 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("zicsr"), 1U);
+ }
+}
+
+TEST(ParseArchString, AcceptsRelaxMixedLetterExtensions) {
+ for (StringRef Input :
+ {"rv32i_zihintntl_m_a_f_d_svinval", "rv32izihintntl_mafdsvinval",
+ "rv32i_zihintntl_mafd_svinval"}) {
+ auto MaybeISAInfo = RISCVISAInfo::parseArchString(Input, true);
+ ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
+ RISCVISAInfo::OrderedExtensionMap Exts = (*MaybeISAInfo)->getExtensions();
+ EXPECT_EQ(Exts.size(), 8UL);
+ EXPECT_EQ(Exts.count("i"), 1U);
+ EXPECT_EQ(Exts.count("m"), 1U);
+ EXPECT_EQ(Exts.count("a"), 1U);
+ EXPECT_EQ(Exts.count("f"), 1U);
+ EXPECT_EQ(Exts.count("d"), 1U);
+ EXPECT_EQ(Exts.count("zihintntl"), 1U);
+ EXPECT_EQ(Exts.count("svinval"), 1U);
+ EXPECT_EQ(Exts.count("zicsr"), 1U);
+ }
+}
+
+TEST(ParseArchString, AcceptsAmbiguousFromRelaxExtensions) {
+ for (StringRef Input : {"rv32i_zba_m", "rv32izba_m", "rv32izba1p0_m2p0"}) {
+ auto MaybeISAInfo = RISCVISAInfo::parseArchString(Input, true);
+ ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
+ RISCVISAInfo::OrderedExtensionMap Exts = (*MaybeISAInfo)->getExtensions();
+ EXPECT_EQ(Exts.size(), 3UL);
+ EXPECT_EQ(Exts.count("i"), 1U);
+ EXPECT_EQ(Exts.count("zba"), 1U);
+ EXPECT_EQ(Exts.count("m"), 1U);
+ }
+ for (StringRef Input :
+ {"rv32ia_zba_m", "rv32iazba_m", "rv32ia2p1zba1p0_m2p0"}) {
+ auto MaybeISAInfo = RISCVISAInfo::parseArchString(Input, true);
+ ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
+ RISCVISAInfo::OrderedExtensionMap Exts = (*MaybeISAInfo)->getExtensions();
+ EXPECT_EQ(Exts.size(), 4UL);
+ EXPECT_EQ(Exts.count("i"), 1U);
+ EXPECT_EQ(Exts.count("zba"), 1U);
+ EXPECT_EQ(Exts.count("m"), 1U);
+ EXPECT_EQ(Exts.count("a"), 1U);
+ }
+}
+
+TEST(ParseArchString,
+ RejectsMultiLetterExtensionFollowBySingleLetterExtensions) {
+ for (StringRef Input : {"rv32izbam", "rv32i_zbam"})
+ EXPECT_EQ(toString(RISCVISAInfo::parseArchString(Input, true).takeError()),
+ "unsupported standard user-level extension 'zbam'");
+ EXPECT_EQ(
+ toString(
+ RISCVISAInfo::parseArchString("rv32i_zba1p0m", true).takeError()),
+ "unsupported standard user-level ...
[truncated]
|
|
Just to check, can someone confirm if gcc is now handling ISA strings in this way too? I do think this is the right direction to go but haven't done a detailed code review of this approach yet. |
|
I am working on GCC part[1], and it's still under review, also @BeMg is working very closely with me :) [1] https://gcc.gnu.org/pipermail/gcc-patches/2024-January/642151.html |
MaskRay
reviewed
Jan 19, 2024
clang/test/Driver/riscv-arch.c
Outdated
| // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32IMADF %s | ||
| // RV32IMADF: error: invalid arch name 'rv32imadf' | ||
| // RUN: %clang --target=riscv32-unknown-elf -march=rv32imadf -### %s \ | ||
| // RUN: -fsyntax-only 2>&1 | FileCheck %s |
There was a problem hiding this comment.
The convention is to indent continuation lines by 2 spaces. Some tests do not follow the convention. You can make them follow the convention as you are modifying lines.
There was a problem hiding this comment.
Add two more space for some RUN LINE.
MaskRay
reviewed
Jan 19, 2024
| // Parse the ISA string containing non-standard user-level | ||
| // extensions, standard supervisor-level extensions and | ||
| // non-standard supervisor-level extensions. | ||
| // These extensions start with 'z', 's', 'x' prefixes, might have a version |
There was a problem hiding this comment.
This comment appears to be removed. Consider retaining it.
There was a problem hiding this comment.
Comment has been restored.
MaskRay
approved these changes
Jan 19, 2024
There was a problem hiding this comment.
GCC patch has just landed: https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643411.html
Perhaps: Follow riscv-non-isa/riscv-toolchain-conventions#14 by dropping the order requirement of |
MaskRay
reviewed
Jan 19, 2024
| } | ||
|
|
||
| TEST(ParseArchString, AcceptsAmbiguousFromRelaxExtensions) { | ||
| for (StringRef Input : {"rv32i_zba_m", "rv32izba_m", "rv32izba1p0_m2p0"}) { |
There was a problem hiding this comment.
Consider another test where i is after zba
There was a problem hiding this comment.
Add three testcases
EXPECT_EQ(
toString(RISCVISAInfo::parseArchString("rv32zba_im", true).takeError()),
"first letter should be 'e', 'i' or 'g'");
EXPECT_EQ(
toString(RISCVISAInfo::parseArchString("rv32izbai_m", true).takeError()),
"unsupported standard user-level extension 'zbai'");
EXPECT_EQ(
toString(RISCVISAInfo::parseArchString("rv32izbaim", true).takeError()),
"unsupported standard user-level extension 'zbaim'");
topperc
reviewed
Jan 23, 2024
llvm/lib/Support/RISCVISAInfo.cpp
Outdated
| @@ -695,6 +695,106 @@ RISCVISAInfo::parseNormalizedArchString(StringRef Arch) { | |||
| return std::move(ISAInfo); | |||
| } | |||
|
|
|||
| static Error splitExtsByUnderscore(StringRef Exts, | |||
| std::vector<std::string> &SplitedExts) { | |||
There was a problem hiding this comment.
SplitExts
Splitted has two ts but I don't think the ed is needed.
llvm/lib/Support/RISCVISAInfo.cpp
Outdated
| @@ -715,6 +815,8 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension, | |||
|
|
|||
| unsigned XLen = HasRV64 ? 64 : 32; | |||
| std::unique_ptr<RISCVISAInfo> ISAInfo(new RISCVISAInfo(XLen)); | |||
| SmallVector<std::string, 8> SeenExts; | |||
There was a problem hiding this comment.
Maybe SeenExts should be a SetVector to make it more efficient to check for duplicates?
Or maybe a MapVector storing the version as the value so we don't need ExtsVersion?
There was a problem hiding this comment.
Use MapVector instead of SmallVector
topperc
reviewed
Jan 24, 2024
llvm/lib/Support/RISCVISAInfo.cpp
Outdated
| } | ||
|
|
||
| // Check if duplicated extension. | ||
| if (!IgnoreUnknown && (SeenExtMap.find(Name.str()) != SeenExtMap.end())) |
There was a problem hiding this comment.
SeenExtMap.contains(Name.str())
llvm/lib/Support/RISCVISAInfo.cpp
Outdated
| RawExt = RawExt.substr(ConsumeLength); | ||
|
|
||
| // Check if duplicated extension. | ||
| if (!IgnoreUnknown && (SeenExtMap.find(Name.str()) != SeenExtMap.end())) |
topperc
reviewed
Jan 25, 2024
BeMg
force-pushed
the
relax-riscv-march-order
branch
from
1873aec to
06ac457
Compare
BeMg
force-pushed
the
relax-riscv-march-order
branch
2 times, most recently
from
4a207be to
a86f982
Compare
BeMg
force-pushed
the
relax-riscv-march-order
branch
from
a86f982 to
0accb78
Compare
joker-eph
added a commit
that referenced
this pull request
Jan 30, 2024
Reverts #78120 Buildbot is broken: llvm/lib/Support/RISCVISAInfo.cpp:910:18: error: call to deleted constructor of 'llvm::Error' return E; ^
|
Sorry, had to revert because this broke a buildbot: |
topperc
pushed a commit
that referenced
this pull request
Jan 30, 2024
With std::move added to fix build bot failure. Original commit message: Follow riscv-non-isa/riscv-toolchain-conventions#14 by dropping the order requirement of `-march`. 1. single-letter extension can be arbitrary order - march=rv32iamdf 2. single-letter extension and multi-letter extension can be mixed - march=rv32i_zihintntl_m_a_f_d_svinval 3. multi-letter extension need seperate the following extension by underscore, otherwise it will be intreprete as one extension. - march=rv32i_zbam -> i,zbam - march=rv32i_zba_m -> i,zba,m
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.
Follow riscv-non-isa/riscv-toolchain-conventions#14 by dropping the order requirement of
-march.