[RISCV] Remove SeenExtMap from RISCVISAInfo::parseArchString. #97506
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
Use the Exts map directly instead of adding to a temporary MapVector first. There are a couple functional change from this. -If an unknown extension is duplicated, we will now print an error for it being unknown instead of an error for it being duplicated. -If an uknown extension is followed by an underscore with no extension after it, we will error for the unknown extension instead of the dangling underscore. These don't seem like serious changes to me. I've updated tests accordingly.
llvmbot
added
clang
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-clang-driver Author: Craig Topper (topperc) ChangesUse the Exts map directly instead of adding to a temporary MapVector first. There are a couple functional change from this. These don't seem like serious changes to me. I've updated tests accordingly. Full diff: https://github.com/llvm/llvm-project/pull/97506.diff 2 Files Affected:
diff --git a/clang/test/Driver/riscv-arch.c b/clang/test/Driver/riscv-arch.c
index ffd92e1f398c4..0f285f7c0033c 100644
--- a/clang/test/Driver/riscv-arch.c
+++ b/clang/test/Driver/riscv-arch.c
@@ -306,7 +306,7 @@
// RUN: not %clang --target=riscv32-unknown-elf -march=rv32ixabc_ -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-XSEP %s
// RV32-XSEP: error: invalid arch name 'rv32ixabc_',
-// RV32-XSEP: extension name missing after separator '_'
+// RV32-XSEP: unsupported non-standard user-level extension 'xabc'
// RUN: not %clang --target=riscv32-unknown-elf -march=rv32ixabc_a -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-PREFIX %s
@@ -318,10 +318,10 @@
// RV32-X-ORDER: error: invalid arch name 'rv32ixdef_sabc',
// RV32-X-ORDER unsupported non-standard user-level extension 'xdef'
-// RUN: not %clang --target=riscv32-unknown-elf -march=rv32ixabc_xabc -### %s \
+// RUN: not %clang --target=riscv32-unknown-elf -march=rv32im_m -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-XDUP %s
-// RV32-XDUP: error: invalid arch name 'rv32ixabc_xabc',
-// RV32-XDUP: duplicated non-standard user-level extension 'xabc'
+// RV32-XDUP: error: invalid arch name 'rv32im_m',
+// RV32-XDUP: duplicated standard user-level extension 'm'
// RUN: not %clang --target=riscv32-unknown-elf -march=rv32ixabc_xdef -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-X-X-INVAL %s
diff --git a/llvm/lib/TargetParser/RISCVISAInfo.cpp b/llvm/lib/TargetParser/RISCVISAInfo.cpp
index b4fd067a1ed7a..5be936a7c642c 100644
--- a/llvm/lib/TargetParser/RISCVISAInfo.cpp
+++ b/llvm/lib/TargetParser/RISCVISAInfo.cpp
@@ -577,9 +577,6 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
"profile name");
std::unique_ptr<RISCVISAInfo> ISAInfo(new RISCVISAInfo(XLen));
- MapVector<std::string, RISCVISAUtils::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
@@ -603,8 +600,7 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
EnableExperimentalExtension, ExperimentalExtensionVersionCheck))
return std::move(E);
- // Postpone AddExtension until end of this function
- SeenExtMap[StringRef(&Baseline, 1).str()] = {Major, Minor};
+ ISAInfo->Exts[std::string(1, Baseline)] = {Major, Minor};
break;
case 'g':
// g expands to extensions in RISCVGImplications.
@@ -618,11 +614,11 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
// No matter which version is given to `g`, we always set imafd to default
// version since the we don't have clear version scheme for that on
// ISA spec.
- for (const auto *Ext : RISCVGImplications) {
+ for (const char *Ext : RISCVGImplications) {
auto Version = findDefaultVersion(Ext);
assert(Version && "Default extension version not found?");
// Postpone AddExtension until end of this function
- SeenExtMap[Ext] = {Version->Major, Version->Minor};
+ ISAInfo->Exts[std::string(Ext)] = {Version->Major, Version->Minor};
}
break;
}
@@ -686,24 +682,20 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
if (Name.size() == 1)
Ext = Ext.substr(ConsumeLength);
- // Check if duplicated extension.
- if (SeenExtMap.contains(Name.str()))
+ if (!RISCVISAInfo::isSupportedExtension(Name))
+ return getStringErrorForInvalidExt(Name);
+
+ // Insert and error for duplicates.
+ if (!ISAInfo->Exts
+ .emplace(Name.str(),
+ RISCVISAUtils::ExtensionVersion{Major, Minor})
+ .second)
return createStringError(errc::invalid_argument,
"duplicated " + Desc + " '" + Name + "'");
- SeenExtMap[Name.str()] = {Major, Minor};
} while (!Ext.empty());
}
- // Check all Extensions are supported.
- for (auto &SeenExtAndVers : SeenExtMap) {
- const std::string &ExtName = SeenExtAndVers.first;
-
- if (!RISCVISAInfo::isSupportedExtension(ExtName))
- return getStringErrorForInvalidExt(ExtName);
- ISAInfo->Exts[ExtName] = SeenExtAndVers.second;
- }
-
return RISCVISAInfo::postProcessAndChecking(std::move(ISAInfo));
}
|
|
@llvm/pr-subscribers-clang Author: Craig Topper (topperc) ChangesUse the Exts map directly instead of adding to a temporary MapVector first. There are a couple functional change from this. These don't seem like serious changes to me. I've updated tests accordingly. Full diff: https://github.com/llvm/llvm-project/pull/97506.diff 2 Files Affected:
diff --git a/clang/test/Driver/riscv-arch.c b/clang/test/Driver/riscv-arch.c
index ffd92e1f398c4..0f285f7c0033c 100644
--- a/clang/test/Driver/riscv-arch.c
+++ b/clang/test/Driver/riscv-arch.c
@@ -306,7 +306,7 @@
// RUN: not %clang --target=riscv32-unknown-elf -march=rv32ixabc_ -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-XSEP %s
// RV32-XSEP: error: invalid arch name 'rv32ixabc_',
-// RV32-XSEP: extension name missing after separator '_'
+// RV32-XSEP: unsupported non-standard user-level extension 'xabc'
// RUN: not %clang --target=riscv32-unknown-elf -march=rv32ixabc_a -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-PREFIX %s
@@ -318,10 +318,10 @@
// RV32-X-ORDER: error: invalid arch name 'rv32ixdef_sabc',
// RV32-X-ORDER unsupported non-standard user-level extension 'xdef'
-// RUN: not %clang --target=riscv32-unknown-elf -march=rv32ixabc_xabc -### %s \
+// RUN: not %clang --target=riscv32-unknown-elf -march=rv32im_m -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-XDUP %s
-// RV32-XDUP: error: invalid arch name 'rv32ixabc_xabc',
-// RV32-XDUP: duplicated non-standard user-level extension 'xabc'
+// RV32-XDUP: error: invalid arch name 'rv32im_m',
+// RV32-XDUP: duplicated standard user-level extension 'm'
// RUN: not %clang --target=riscv32-unknown-elf -march=rv32ixabc_xdef -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-X-X-INVAL %s
diff --git a/llvm/lib/TargetParser/RISCVISAInfo.cpp b/llvm/lib/TargetParser/RISCVISAInfo.cpp
index b4fd067a1ed7a..5be936a7c642c 100644
--- a/llvm/lib/TargetParser/RISCVISAInfo.cpp
+++ b/llvm/lib/TargetParser/RISCVISAInfo.cpp
@@ -577,9 +577,6 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
"profile name");
std::unique_ptr<RISCVISAInfo> ISAInfo(new RISCVISAInfo(XLen));
- MapVector<std::string, RISCVISAUtils::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
@@ -603,8 +600,7 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
EnableExperimentalExtension, ExperimentalExtensionVersionCheck))
return std::move(E);
- // Postpone AddExtension until end of this function
- SeenExtMap[StringRef(&Baseline, 1).str()] = {Major, Minor};
+ ISAInfo->Exts[std::string(1, Baseline)] = {Major, Minor};
break;
case 'g':
// g expands to extensions in RISCVGImplications.
@@ -618,11 +614,11 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
// No matter which version is given to `g`, we always set imafd to default
// version since the we don't have clear version scheme for that on
// ISA spec.
- for (const auto *Ext : RISCVGImplications) {
+ for (const char *Ext : RISCVGImplications) {
auto Version = findDefaultVersion(Ext);
assert(Version && "Default extension version not found?");
// Postpone AddExtension until end of this function
- SeenExtMap[Ext] = {Version->Major, Version->Minor};
+ ISAInfo->Exts[std::string(Ext)] = {Version->Major, Version->Minor};
}
break;
}
@@ -686,24 +682,20 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
if (Name.size() == 1)
Ext = Ext.substr(ConsumeLength);
- // Check if duplicated extension.
- if (SeenExtMap.contains(Name.str()))
+ if (!RISCVISAInfo::isSupportedExtension(Name))
+ return getStringErrorForInvalidExt(Name);
+
+ // Insert and error for duplicates.
+ if (!ISAInfo->Exts
+ .emplace(Name.str(),
+ RISCVISAUtils::ExtensionVersion{Major, Minor})
+ .second)
return createStringError(errc::invalid_argument,
"duplicated " + Desc + " '" + Name + "'");
- SeenExtMap[Name.str()] = {Major, Minor};
} while (!Ext.empty());
}
- // Check all Extensions are supported.
- for (auto &SeenExtAndVers : SeenExtMap) {
- const std::string &ExtName = SeenExtAndVers.first;
-
- if (!RISCVISAInfo::isSupportedExtension(ExtName))
- return getStringErrorForInvalidExt(ExtName);
- ISAInfo->Exts[ExtName] = SeenExtAndVers.second;
- }
-
return RISCVISAInfo::postProcessAndChecking(std::move(ISAInfo));
}
|
wangpc-pp
reviewed
Jul 3, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/66/builds/978 Here is the relevant piece of the build log for the reference: |
kbluck
pushed a commit
to kbluck/llvm-project
that referenced
this pull request
Jul 6, 2024
…7506) Use the Exts map directly instead of adding to a temporary MapVector first. There are a couple functional change from this. -If an unknown extension is duplicated, we will now print an error for it being unknown instead of an error for it being duplicated. -If an unknown extension is followed by an underscore with no extension after it, we will error for the unknown extension instead of the dangling underscore. These don't seem like serious changes to me. I've updated tests accordingly.
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.
Use the Exts map directly instead of adding to a temporary MapVector first.
There are a couple functional change from this.
-If an unknown extension is duplicated, we will now print an error for it being unknown instead of an error for it being duplicated. -If an uknown extension is followed by an underscore with no extension after it, we will error for the unknown extension instead of the dangling underscore.
These don't seem like serious changes to me. I've updated tests accordingly.