[llvm] [clang] Revert "[RISCV] Relax march string order constraint" (PR #79976)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Jan 30 02:33:20 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-backend-risc-v
@llvm/pr-subscribers-clang
Author: Mehdi Amini (joker-eph)
<details>
<summary>Changes</summary>
Reverts llvm/llvm-project#<!-- -->78120
Buildbot is broken:
llvm/lib/Support/RISCVISAInfo.cpp:910:18: error: call to deleted constructor of 'llvm::Error'
return E;
^
---
Patch is 22.12 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/79976.diff
3 Files Affected:
- (modified) clang/test/Driver/riscv-arch.c (+8-6)
- (modified) llvm/lib/Support/RISCVISAInfo.cpp (+145-157)
- (modified) llvm/unittests/Support/RISCVISAInfoTest.cpp (+20-83)
``````````diff
diff --git a/clang/test/Driver/riscv-arch.c b/clang/test/Driver/riscv-arch.c
index c9e984e07cbe..0ac81ea982f1 100644
--- a/clang/test/Driver/riscv-arch.c
+++ b/clang/test/Driver/riscv-arch.c
@@ -156,8 +156,9 @@
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32L %s
// RV32L: error: invalid arch name 'rv32l'
-// RUN: %clang --target=riscv32-unknown-elf -march=rv32imadf -### %s \
-// RUN: -fsyntax-only 2>&1 | FileCheck %s
+// 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: not %clang --target=riscv32-unknown-elf -march=rv32imm -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32IMM %s
@@ -183,8 +184,9 @@
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64L %s
// RV64L: error: invalid arch name 'rv64l'
-// RUN: %clang --target=riscv64-unknown-elf -march=rv64imadf -### %s \
-// RUN: -fsyntax-only 2>&1 | FileCheck %s
+// 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: not %clang --target=riscv64-unknown-elf -march=rv64imm -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV64IMM %s
@@ -214,7 +216,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: unsupported standard user-level extension 'q'
+// RV32-ORDER: standard user-level extension not given in canonical order 'q'
// RUN: not %clang --target=riscv32-unknown-elf -march=rv32izvl64b -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-ZVL64B-ER %s
@@ -316,7 +318,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: unsupported non-standard user-level extension 'xabc'
+// RV32-PREFIX: invalid extension prefix 'a'
// 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 050253f78399..c46d76da962c 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -7,7 +7,6 @@
//===----------------------------------------------------------------------===//
#include "llvm/Support/RISCVISAInfo.h"
-#include "llvm/ADT/MapVector.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/StringExtras.h"
@@ -704,106 +703,6 @@ RISCVISAInfo::parseNormalizedArchString(StringRef Arch) {
return std::move(ISAInfo);
}
-static Error splitExtsByUnderscore(StringRef Exts,
- std::vector<std::string> &SplitExts) {
- 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 '_'");
-
- SplitExts.push_back(Ext.str());
- }
- return Error::success();
-}
-
-static Error processMultiLetterExtension(
- StringRef RawExt,
- MapVector<std::string, RISCVISAInfo::ExtensionVersion,
- std::map<std::string, unsigned>> &SeenExtMap,
- 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 && SeenExtMap.contains(Name.str()))
- return createStringError(errc::invalid_argument, "duplicated %s '%s'",
- Desc.str().c_str(), Name.str().c_str());
-
- if (IgnoreUnknown && !RISCVISAInfo::isSupportedExtension(Name))
- return Error::success();
-
- SeenExtMap[Name.str()] = {Major, Minor};
- return Error::success();
-}
-
-static Error processSingleLetterExtension(
- StringRef &RawExt,
- MapVector<std::string, RISCVISAInfo::ExtensionVersion,
- std::map<std::string, unsigned>> &SeenExtMap,
- 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 && SeenExtMap.contains(Name.str()))
- return createStringError(errc::invalid_argument,
- "duplicated standard user-level extension '%s'",
- Name.str().c_str());
-
- if (IgnoreUnknown && !RISCVISAInfo::isSupportedExtension(Name))
- return Error::success();
-
- SeenExtMap[Name.str()] = {Major, Minor};
- return Error::success();
-}
-
llvm::Expected<std::unique_ptr<RISCVISAInfo>>
RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
bool ExperimentalExtensionVersionCheck,
@@ -824,9 +723,6 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
unsigned XLen = HasRV64 ? 64 : 32;
std::unique_ptr<RISCVISAInfo> ISAInfo(new RISCVISAInfo(XLen));
- MapVector<std::string, RISCVISAInfo::ExtensionVersion,
- std::map<std::string, unsigned>>
- SeenExtMap;
// The canonical order specified in ISA manual.
// Ref: Table 22.1 in RISC-V User-Level ISA V2.2
@@ -857,6 +753,17 @@ 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.
@@ -866,10 +773,9 @@ 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)) {
- // Postpone AddExtension until end of this function
- SeenExtMap[Ext] = {Version->Major, Version->Minor};
- } else
+ if (auto Version = findDefaultVersion(Ext))
+ ISAInfo->addExtension(Ext, *Version);
+ else
llvm_unreachable("Default extension version not found?");
}
} else {
@@ -887,8 +793,7 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
Minor = Version->Minor;
}
- // Postpone AddExtension until end of this function
- SeenExtMap[StringRef(&Baseline, 1).str()] = {Major, Minor};
+ ISAInfo->addExtension(StringRef(&Baseline, 1), {Major, Minor});
}
// Consume the base ISA version number and any '_' between rvxxx and the
@@ -896,60 +801,143 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
Exts = Exts.drop_front(ConsumeLength);
Exts.consume_front("_");
- std::vector<std::string> SplittedExts;
- if (auto E = splitExtsByUnderscore(Exts, SplittedExts))
- return std::move(E);
-
- for (auto &Ext : SplittedExts) {
- StringRef CurrExt = Ext;
- while (!CurrExt.empty()) {
- if (AllStdExts.contains(CurrExt.front())) {
- if (auto E = processSingleLetterExtension(
- CurrExt, SeenExtMap, IgnoreUnknown, EnableExperimentalExtension,
- ExperimentalExtensionVersionCheck))
- return E;
- } else if (CurrExt.front() == 'z' || CurrExt.front() == 's' ||
- CurrExt.front() == 'x') {
- // 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.
- if (auto E = processMultiLetterExtension(
- CurrExt, SeenExtMap, 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,
- "invalid standard user-level extension '%c'",
- CurrExt.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 '" +
+ Twine(C) + "'");
+ }
+
+ return createStringError(errc::invalid_argument,
+ "invalid standard user-level extension '" +
+ Twine(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 '" +
+ Twine(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());
}
- // Check all Extensions are supported.
- for (auto &SeenExtAndVers : SeenExtMap) {
- const std::string &ExtName = SeenExtAndVers.first;
- RISCVISAInfo::ExtensionVersion ExtVers = SeenExtAndVers.second;
+ // 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, '_');
- if (!RISCVISAInfo::isSupportedExtension(ExtName)) {
- if (ExtName.size() == 1) {
- return createStringError(
- errc::invalid_argument,
- "unsupported standard user-level extension '%s'", ExtName.c_str());
+ 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())
+ return createStringError(errc::invalid_argument,
+ Desc + " name missing after '" + Type + "'");
+
+ 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);
}
- return createStringError(errc::invalid_argument, "unsupported %s '%s'",
- getExtensionTypeDesc(ExtName).str().c_str(),
- ExtName.c_str());
+
+ // Check if duplicated extension.
+ if (!IgnoreUnknown && llvm::is_contained(AllExts, Name))
+ return createStringError(errc::invalid_argument,
+ "duplicated " + Desc + " '" + Name + "'");
+
+ 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 " + Desc + " '" + Ext + "'");
}
- ISAInfo->addExtension(ExtName, ExtVers);
}
return RISCVISAInfo::postProcessAndChecking(std::move(ISAInfo));
diff --git a/llvm/unittests/Support/RISCVISAInfoTest.cpp b/llvm/unittests/Support/RISCVISAInfoTest.cpp
index f739c26b6880..f03ccecfae79 100644
--- a/llvm/unittests/Support/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/Support/RISCVISAInfoTest.cpp
@@ -203,6 +203,24 @@ 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'");
@@ -319,91 +337,10 @@ 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(I...
[truncated]
``````````
</details>
https://github.com/llvm/llvm-project/pull/79976
More information about the cfe-commits
mailing list