[RISCV] Make RequiredExtensions for intrinsics scalable to more than 32 extensions. NFC #132895
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
…32 extensions. NFC We have more than 32 extensions in our downstream and had to change this type from uint32_t to uint64_t. To simplify our downstream and make the code more flexible, I propose to make it an array of uint32_t that we can size based on the number of extensions. I really wanted to use std::bitset, but we have to print the bits to a .inc file which can't easily be done with std::bitset.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-risc-v Author: Craig Topper (topperc) ChangesWe have more than 32 extensions in our downstream and had to change this type from uint32_t to uint64_t. To simplify our downstream and make the code more flexible, I propose to make it an array of uint32_t that we can size based on the number of extensions. I really wanted to use std::bitset, but we have to print the bits to a .inc file which can't easily be done with std::bitset. Full diff: https://github.com/llvm/llvm-project/pull/132895.diff 4 Files Affected:
diff --git a/clang/include/clang/Support/RISCVVIntrinsicUtils.h b/clang/include/clang/Support/RISCVVIntrinsicUtils.h
index 4819fc144f4dc..d8d7d61fdc59c 100644
--- a/clang/include/clang/Support/RISCVVIntrinsicUtils.h
+++ b/clang/include/clang/Support/RISCVVIntrinsicUtils.h
@@ -484,29 +484,27 @@ class RVVIntrinsic {
// RVVRequire should be sync'ed with target features, but only
// required features used in riscv_vector.td.
enum RVVRequire : uint32_t {
- RVV_REQ_None = 0,
- RVV_REQ_RV64 = 1 << 0,
- RVV_REQ_Zvfhmin = 1 << 1,
- RVV_REQ_Xsfvcp = 1 << 2,
- RVV_REQ_Xsfvfnrclipxfqf = 1 << 3,
- RVV_REQ_Xsfvfwmaccqqq = 1 << 4,
- RVV_REQ_Xsfvqmaccdod = 1 << 5,
- RVV_REQ_Xsfvqmaccqoq = 1 << 6,
- RVV_REQ_Zvbb = 1 << 7,
- RVV_REQ_Zvbc = 1 << 8,
- RVV_REQ_Zvkb = 1 << 9,
- RVV_REQ_Zvkg = 1 << 10,
- RVV_REQ_Zvkned = 1 << 11,
- RVV_REQ_Zvknha = 1 << 12,
- RVV_REQ_Zvknhb = 1 << 13,
- RVV_REQ_Zvksed = 1 << 14,
- RVV_REQ_Zvksh = 1 << 15,
- RVV_REQ_Zvfbfwma = 1 << 16,
- RVV_REQ_Zvfbfmin = 1 << 17,
- RVV_REQ_Zvfh = 1 << 18,
- RVV_REQ_Experimental = 1 << 19,
-
- LLVM_MARK_AS_BITMASK_ENUM(RVV_REQ_Experimental)
+ RVV_REQ_RV64,
+ RVV_REQ_Zvfhmin,
+ RVV_REQ_Xsfvcp,
+ RVV_REQ_Xsfvfnrclipxfqf,
+ RVV_REQ_Xsfvfwmaccqqq,
+ RVV_REQ_Xsfvqmaccdod,
+ RVV_REQ_Xsfvqmaccqoq,
+ RVV_REQ_Zvbb,
+ RVV_REQ_Zvbc,
+ RVV_REQ_Zvkb,
+ RVV_REQ_Zvkg,
+ RVV_REQ_Zvkned,
+ RVV_REQ_Zvknha,
+ RVV_REQ_Zvknhb,
+ RVV_REQ_Zvksed,
+ RVV_REQ_Zvksh,
+ RVV_REQ_Zvfbfwma,
+ RVV_REQ_Zvfbfmin,
+ RVV_REQ_Zvfh,
+ RVV_REQ_Experimental,
+ RVV_REQ_NUM,
};
// Raw RVV intrinsic info, used to expand later.
@@ -519,6 +517,9 @@ struct RVVIntrinsicRecord {
// e.g. vadd
const char *OverloadedName;
+ // Required target features for this intrinsic.
+ uint32_t RequiredExtensions[(RVV_REQ_NUM + 31) / 32];
+
// Prototype for this intrinsic, index of RVVSignatureTable.
uint16_t PrototypeIndex;
@@ -537,9 +538,6 @@ struct RVVIntrinsicRecord {
// Length of overloaded intrinsic suffix.
uint8_t OverloadedSuffixSize;
- // Required target features for this intrinsic.
- uint32_t RequiredExtensions;
-
// Supported type, mask of BasicType.
uint8_t TypeRangeMask;
diff --git a/clang/lib/Sema/SemaRISCV.cpp b/clang/lib/Sema/SemaRISCV.cpp
index f23827d566610..746609604d1ba 100644
--- a/clang/lib/Sema/SemaRISCV.cpp
+++ b/clang/lib/Sema/SemaRISCV.cpp
@@ -206,7 +206,7 @@ class RISCVIntrinsicManagerImpl : public sema::RISCVIntrinsicManager {
void RISCVIntrinsicManagerImpl::ConstructRVVIntrinsics(
ArrayRef<RVVIntrinsicRecord> Recs, IntrinsicKind K) {
const TargetInfo &TI = Context.getTargetInfo();
- static const std::pair<const char *, RVVRequire> FeatureCheckList[] = {
+ static const std::pair<const char *, unsigned> FeatureCheckList[] = {
{"64bit", RVV_REQ_RV64},
{"xsfvcp", RVV_REQ_Xsfvcp},
{"xsfvfnrclipxfqf", RVV_REQ_Xsfvfnrclipxfqf},
@@ -232,7 +232,8 @@ void RISCVIntrinsicManagerImpl::ConstructRVVIntrinsics(
for (auto &Record : Recs) {
// Check requirements.
if (llvm::any_of(FeatureCheckList, [&](const auto &Item) {
- return (Record.RequiredExtensions & Item.second) == Item.second &&
+ return ((Record.RequiredExtensions[Item.second / 32] &
+ (1U << (Item.second % 32))) != 0) &&
!TI.hasFeature(Item.first);
}))
continue;
diff --git a/clang/lib/Support/RISCVVIntrinsicUtils.cpp b/clang/lib/Support/RISCVVIntrinsicUtils.cpp
index 091e35f47fe8e..e44fbb0181830 100644
--- a/clang/lib/Support/RISCVVIntrinsicUtils.cpp
+++ b/clang/lib/Support/RISCVVIntrinsicUtils.cpp
@@ -1204,13 +1204,16 @@ raw_ostream &operator<<(raw_ostream &OS, const RVVIntrinsicRecord &Record) {
OS << "nullptr,";
else
OS << "\"" << Record.OverloadedName << "\",";
+ OS << "{";
+ for (uint32_t Exts : Record.RequiredExtensions)
+ OS << Exts << ',';
+ OS << "},";
OS << Record.PrototypeIndex << ",";
OS << Record.SuffixIndex << ",";
OS << Record.OverloadedSuffixIndex << ",";
OS << (int)Record.PrototypeLength << ",";
OS << (int)Record.SuffixLength << ",";
OS << (int)Record.OverloadedSuffixSize << ",";
- OS << Record.RequiredExtensions << ",";
OS << (int)Record.TypeRangeMask << ",";
OS << (int)Record.Log2LMULMask << ",";
OS << (int)Record.NF << ",";
diff --git a/clang/utils/TableGen/RISCVVEmitter.cpp b/clang/utils/TableGen/RISCVVEmitter.cpp
index 0cdde20060b63..42cb0508ecb60 100644
--- a/clang/utils/TableGen/RISCVVEmitter.cpp
+++ b/clang/utils/TableGen/RISCVVEmitter.cpp
@@ -45,7 +45,7 @@ struct SemaRecord {
unsigned Log2LMULMask;
// Required extensions for this intrinsic.
- uint32_t RequiredExtensions;
+ uint32_t RequiredExtensions[(RVV_REQ_Experimental + 31) / 32];
// Prototype for this intrinsic.
SmallVector<PrototypeDescriptor> Prototype;
@@ -769,9 +769,9 @@ void RVVEmitter::createRVVIntrinsics(
SR.Log2LMULMask = Log2LMULMask;
- SR.RequiredExtensions = 0;
+ memset(SR.RequiredExtensions, 0, sizeof(SR.RequiredExtensions));
for (auto RequiredFeature : RequiredFeatures) {
- RVVRequire RequireExt =
+ unsigned RequireExt =
StringSwitch<RVVRequire>(RequiredFeature)
.Case("RV64", RVV_REQ_RV64)
.Case("Zvfhmin", RVV_REQ_Zvfhmin)
@@ -792,10 +792,8 @@ void RVVEmitter::createRVVIntrinsics(
.Case("Zvfbfwma", RVV_REQ_Zvfbfwma)
.Case("Zvfbfmin", RVV_REQ_Zvfbfmin)
.Case("Zvfh", RVV_REQ_Zvfh)
- .Case("Experimental", RVV_REQ_Experimental)
- .Default(RVV_REQ_None);
- assert(RequireExt != RVV_REQ_None && "Unrecognized required feature?");
- SR.RequiredExtensions |= RequireExt;
+ .Case("Experimental", RVV_REQ_Experimental);
+ SR.RequiredExtensions[RequireExt / 32] |= 1U << (RequireExt % 32);
}
SR.NF = NF;
@@ -839,7 +837,8 @@ void RVVEmitter::createRVVIntrinsicRecords(std::vector<RVVIntrinsicRecord> &Out,
R.PrototypeLength = SR.Prototype.size();
R.SuffixLength = SR.Suffix.size();
R.OverloadedSuffixSize = SR.OverloadedSuffix.size();
- R.RequiredExtensions = SR.RequiredExtensions;
+ memcpy(R.RequiredExtensions, SR.RequiredExtensions,
+ sizeof(R.RequiredExtensions));
R.TypeRangeMask = SR.TypeRangeMask;
R.Log2LMULMask = SR.Log2LMULMask;
R.NF = SR.NF;
|
wangpc-pp
reviewed
Mar 25, 2025
| @@ -519,6 +517,9 @@ struct RVVIntrinsicRecord { | |||
| // e.g. vadd | |||
| const char *OverloadedName; | |||
|
|
|||
| // Required target features for this intrinsic. | |||
| uint32_t RequiredExtensions[(RVV_REQ_NUM + 31) / 32]; | |||
There was a problem hiding this comment.
I wonder if we can use FeatureBitset here and reuse extension enums so that we don't need to define RVVRequire.
There was a problem hiding this comment.
FeatureBits is very large and will waste a lot of space.
We also don't have access to the RISCV feature enum if RISCV isn't passed to cmake's LLVM_TARGETS_TO_BUILD.
|
Do you hit the same problem with |
Yeah. There's no way to access the underlying array. The constructor for llvm::Bitset takes a list of bits. Maybe we could print out the bit positions instead of the array? |
lenary
approved these changes
Mar 26, 2025
Yeah I prefer to use |
The generated code is completely unreadable right now for other reasons. An excerpt. |
I just found that, I think we should make it readable. I'll do that. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/13969 Here is the relevant piece of the build log for the reference |
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.
We have more than 32 extensions in our downstream and had to change this type from uint32_t to uint64_t.
To simplify our downstream and make the code more flexible, I propose to make it an array of uint32_t that we can size based on the number of extensions. I really wanted to use std::bitset, but we have to print the bits to a .inc file which can't easily be done with std::bitset.