[RISCV]Enable framework to resolve encoding conflicts among vendor-specific CSRs #97287
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
|
@llvm/pr-subscribers-backend-risc-v Author: Garvit Gupta (quic-garvgupt) ChangesThis PR is a follow-up of PR #96174 which added the frameowrk to resolve Full diff: https://github.com/llvm/llvm-project/pull/97287.diff 6 Files Affected:
diff --git a/llvm/include/llvm/TableGen/SearchableTable.td b/llvm/include/llvm/TableGen/SearchableTable.td
index 9dddd5e578ff1..b9dfda0e23b73 100644
--- a/llvm/include/llvm/TableGen/SearchableTable.td
+++ b/llvm/include/llvm/TableGen/SearchableTable.td
@@ -114,6 +114,9 @@ class GenericTable {
// See SearchIndex.EarlyOut
bit PrimaryKeyEarlyOut = false;
+
+ // See SearchIndex.ReturnRange
+ bit PrimaryKeyReturnRange = false;
}
// Define a record derived from this class to generate an additional search
@@ -135,6 +138,11 @@ class SearchIndex {
//
// Can only be used when the first field is an integral (non-string) type.
bit EarlyOut = false;
+
+ // If true, will generate a different function signature which will return
+ // a std::pair<> of iterators pointing to start and end value of the range
+ // e.g. lookupSysRegByEncoding returns multiple CSRs for same encoding.
+ bit ReturnRange = false;
}
// Legacy table type with integrated enum.
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 8ac1cdf0a7a9c..b61224922d45d 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -1860,11 +1860,18 @@ ParseStatus RISCVAsmParser::parseCSRSystemRegister(OperandVector &Operands) {
if (CE) {
int64_t Imm = CE->getValue();
if (isUInt<12>(Imm)) {
- auto SysReg = RISCVSysReg::lookupSysRegByEncoding(Imm);
- // Accept an immediate representing a named or un-named Sys Reg
- // if the range is valid, regardless of the required features.
- Operands.push_back(
- RISCVOperand::createSysReg(SysReg ? SysReg->Name : "", S, Imm));
+ auto Range = RISCVSysReg::lookupSysRegByEncoding(Imm);
+ // Accept an immediate representing a named Sys Reg if it satisfies the
+ // the requried features.
+ for (auto It : Range) {
+ if (It.haveRequiredFeatures(STI->getFeatureBits())) {
+ Operands.push_back(RISCVOperand::createSysReg(It.Name, S, Imm));
+ return ParseStatus::Success;
+ }
+ }
+ // Accept an immediate representing an un-named Sys Reg if the range is
+ // valid, regardless of the required features.
+ Operands.push_back(RISCVOperand::createSysReg("", S, Imm));
return ParseStatus::Success;
}
}
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
index 48b669c78cade..19cfde4fbd051 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
@@ -121,11 +121,14 @@ void RISCVInstPrinter::printCSRSystemRegister(const MCInst *MI, unsigned OpNo,
const MCSubtargetInfo &STI,
raw_ostream &O) {
unsigned Imm = MI->getOperand(OpNo).getImm();
- auto SysReg = RISCVSysReg::lookupSysRegByEncoding(Imm);
- if (SysReg && SysReg->haveRequiredFeatures(STI.getFeatureBits()))
- markup(O, Markup::Register) << SysReg->Name;
- else
- markup(O, Markup::Register) << formatImm(Imm);
+ auto Range = RISCVSysReg::lookupSysRegByEncoding(Imm);
+ for(auto It : Range){
+ if (It.haveRequiredFeatures(STI.getFeatureBits())) {
+ markup(O, Markup::Register) << It.Name;
+ return;
+ }
+ }
+ markup(O, Markup::Register) << formatImm(Imm);
}
void RISCVInstPrinter::printFenceArg(const MCInst *MI, unsigned OpNo,
diff --git a/llvm/lib/Target/RISCV/RISCVSystemOperands.td b/llvm/lib/Target/RISCV/RISCVSystemOperands.td
index db840b3027492..a836227e18957 100644
--- a/llvm/lib/Target/RISCV/RISCVSystemOperands.td
+++ b/llvm/lib/Target/RISCV/RISCVSystemOperands.td
@@ -49,6 +49,7 @@ def SysRegsList : GenericTable {
let PrimaryKey = [ "Encoding" ];
let PrimaryKeyName = "lookupSysRegByEncoding";
+ let PrimaryKeyReturnRange = true;
}
def lookupSysRegByName : SearchIndex {
diff --git a/llvm/test/TableGen/EncodingConflict.td b/llvm/test/TableGen/EncodingConflict.td
new file mode 100644
index 0000000000000..3dfb389f6c716
--- /dev/null
+++ b/llvm/test/TableGen/EncodingConflict.td
@@ -0,0 +1,109 @@
+// RUN: llvm-tblgen -gen-searchable-tables -I %p/../../include %s | FileCheck %s
+
+include "llvm/TableGen/SearchableTable.td"
+
+class SysReg<string name, bits<12> op> {
+ string Name = name;
+ bits<12> Encoding = op;
+ code FeaturesRequired = [{ {} }];
+}
+
+def List1 : GenericTable {
+ let FilterClass = "SysReg";
+ let Fields = [
+ "Name", "Encoding", "FeaturesRequired",
+ ];
+
+ let PrimaryKey = [ "Encoding" ];
+ let PrimaryKeyName = "lookupSysRegByEncoding";
+ let PrimaryKeyReturnRange = true;
+}
+
+
+let FeaturesRequired = [{ {Feature1} }] in {
+def : SysReg<"csr1", 0x7C0>;
+}
+
+let FeaturesRequired = [{ {Feature2} }] in {
+def : SysReg<"csr2", 0x7C0>;
+}
+
+// CHECK: #ifdef GET_List1_DECL
+// CHECK-NEXT: llvm::iterator_range<const SysReg *>lookupSysRegByEncoding(uint16_t Encoding);
+// CHECK-NEXT: #endif
+
+// CHECK: #ifdef GET_List1_IMPL
+// CHECK-NEXT: constexpr SysReg List1[] = {
+// CHECK-NEXT: { "csr1", 0x7C0, {Feature1} }, // 0
+// CHECK-NEXT: { "csr2", 0x7C0, {Feature2} }, // 1
+// CHECK-NEXT: };
+
+// CHECK: llvm::iterator_range<const SysReg *>lookupSysRegByEncoding(uint16_t Encoding) {
+// CHECK-NEXT: SysReg Key;
+// CHECK-NEXT: Key.Encoding = Encoding;
+// CHECK-NEXT: auto Table = ArrayRef(List1);
+// CHECK-NEXT: auto It = std::equal_range(Table.begin(), Table.end(), Key,
+// CHECK-NEXT: [](const SysReg &LHS, const SysReg &RHS) {
+// CHECK-NEXT: if (LHS.Encoding < RHS.Encoding)
+// CHECK-NEXT: return true;
+// CHECK-NEXT: if (LHS.Encoding > RHS.Encoding)
+// CHECK-NEXT: return false;
+// CHECK-NEXT: return false;
+// CHECK-NEXT: });
+
+// CHECK: return llvm::make_range(It.first, It.second);
+// CHECK-NEXT: }
+// CHECK-NEXT: #endif
+
+def List2 : GenericTable {
+ let FilterClass = "SysReg";
+ let Fields = [
+ "Name", "Encoding", "FeaturesRequired",
+ ];
+}
+
+def lookupSysRegByName : SearchIndex {
+ let Table = List2;
+ let Key = [ "Name" ];
+ let ReturnRange = true;
+}
+
+// CHECK: #ifdef GET_List2_DECL
+// CHECK-NEXT: llvm::iterator_range<const SysReg *>lookupSysRegByName(StringRef Name);
+// CHECK-NEXT: #endif
+
+// CHECK: #ifdef GET_List2_IMPL
+// CHECK-NEXT: constexpr SysReg List2[] = {
+// CHECK-NEXT: { "csr1", 0x7C0, {Feature1} }, // 0
+// CHECK-NEXT: { "csr2", 0x7C0, {Feature2} }, // 1
+// CHECK-NEXT: };
+
+// CHECK: llvm::iterator_range<const SysReg *>lookupSysRegByName(StringRef Name) {
+// CHECK-NEXT: struct IndexType {
+// CHECK-NEXT: const char * Name;
+// CHECK-NEXT: unsigned _index;
+// CHECK-NEXT: };
+// CHECK-NEXT: static const struct IndexType Index[] = {
+// CHECK-NEXT: { "CSR1", 0 },
+// CHECK-NEXT: { "CSR2", 1 },
+// CHECK-NEXT: };
+
+// CHECK: IndexType Key;
+// CHECK-NEXT: Key.Name = Name.upper();
+// CHECK-NEXT: auto Table = ArrayRef(Index);
+// CHECK-NEXT: auto It = std::equal_range(Table.begin(), Table.end(), Key,
+// CHECK-NEXT: [](const IndexType &LHS, const IndexType &RHS) {
+// CHECK-NEXT: int CmpName = StringRef(LHS.Name).compare(RHS.Name);
+// CHECK-NEXT: if (CmpName < 0) return true;
+// CHECK-NEXT: if (CmpName > 0) return false;
+// CHECK-NEXT: return false;
+// CHECK-NEXT: });
+
+// CHECK: return llvm::make_range(It.first, It.second);
+// CHECK-NEXT: }
+// CHECK-NEXT: #endif
+
+// CHECK: #undef GET_List1_DECL
+// CHECK-NEXT: #undef GET_List1_IMPL
+// CHECK-NEXT: #undef GET_List2_DECL
+// CHECK-NEXT: #undef GET_List2_IMPL
\ No newline at end of file
diff --git a/llvm/utils/TableGen/SearchableTableEmitter.cpp b/llvm/utils/TableGen/SearchableTableEmitter.cpp
index 48ee23db957de..7cab1d90b3c9b 100644
--- a/llvm/utils/TableGen/SearchableTableEmitter.cpp
+++ b/llvm/utils/TableGen/SearchableTableEmitter.cpp
@@ -68,6 +68,7 @@ struct SearchIndex {
SMLoc Loc; // Source location of PrimaryKey or Key field definition.
SmallVector<GenericField, 1> Fields;
bool EarlyOut = false;
+ bool ReturnRange = false;
};
struct GenericTable {
@@ -190,15 +191,18 @@ class SearchableTableEmitter {
void emitGenericTable(const GenericTable &Table, raw_ostream &OS);
void emitGenericEnum(const GenericEnum &Enum, raw_ostream &OS);
void emitLookupDeclaration(const GenericTable &Table,
- const SearchIndex &Index, raw_ostream &OS);
+ const SearchIndex &Index, bool ShouldReturnRange,
+ raw_ostream &OS);
void emitLookupFunction(const GenericTable &Table, const SearchIndex &Index,
- bool IsPrimary, raw_ostream &OS);
+ bool IsPrimary, bool ShouldReturnRange,
+ raw_ostream &OS);
void emitIfdef(StringRef Guard, raw_ostream &OS);
bool parseFieldType(GenericField &Field, Init *II);
std::unique_ptr<SearchIndex>
parseSearchIndex(GenericTable &Table, const RecordVal *RecVal, StringRef Name,
- const std::vector<StringRef> &Key, bool EarlyOut);
+ const std::vector<StringRef> &Key, bool EarlyOut,
+ bool ReturnRange);
void collectEnumEntries(GenericEnum &Enum, StringRef NameField,
StringRef ValueField,
const std::vector<Record *> &Items);
@@ -319,9 +323,10 @@ void SearchableTableEmitter::emitGenericEnum(const GenericEnum &Enum,
void SearchableTableEmitter::emitLookupFunction(const GenericTable &Table,
const SearchIndex &Index,
bool IsPrimary,
+ bool ShouldReturnRange,
raw_ostream &OS) {
OS << "\n";
- emitLookupDeclaration(Table, Index, OS);
+ emitLookupDeclaration(Table, Index, ShouldReturnRange, OS);
OS << " {\n";
std::vector<Record *> IndexRowsStorage;
@@ -426,16 +431,24 @@ void SearchableTableEmitter::emitLookupFunction(const GenericTable &Table,
OS << " return nullptr;\n\n";
}
- OS << " struct KeyType {\n";
- for (const auto &Field : Index.Fields) {
- OS << " " << searchableFieldType(Table, Index, Field, TypeInTempStruct)
- << " " << Field.Name << ";\n";
+ if (ShouldReturnRange)
+ OS << " " << IndexTypeName << " Key;\n";
+ else {
+ OS << " struct KeyType {\n";
+ for (const auto &Field : Index.Fields) {
+ OS << " " << searchableFieldType(Table, Index, Field, TypeInTempStruct)
+ << " " << Field.Name << ";\n";
+ }
+ OS << " };\n";
+ OS << " KeyType Key = {";
}
- OS << " };\n";
- OS << " KeyType Key = {";
+
ListSeparator LS;
for (const auto &Field : Index.Fields) {
- OS << LS << Field.Name;
+ if (ShouldReturnRange)
+ OS << " Key." << Field.Name << " = " << Field.Name;
+ else
+ OS << LS << Field.Name;
if (isa<StringRecTy>(Field.RecType)) {
OS << ".upper()";
if (IsPrimary)
@@ -445,12 +458,21 @@ void SearchableTableEmitter::emitLookupFunction(const GenericTable &Table,
"case-insensitive comparison of field '" +
Field.Name + "'");
}
+ if (ShouldReturnRange)
+ OS << ";\n";
}
- OS << "};\n";
+ if (!ShouldReturnRange)
+ OS << "};\n";
OS << " auto Table = ArrayRef(" << IndexName << ");\n";
- OS << " auto Idx = std::lower_bound(Table.begin(), Table.end(), Key,\n";
- OS << " [](const " << IndexTypeName << " &LHS, const KeyType &RHS) {\n";
+ if (ShouldReturnRange) {
+ OS << " auto It = std::equal_range(Table.begin(), Table.end(), Key,\n";
+ OS << " [](const " << IndexTypeName << " &LHS, const " << IndexTypeName
+ << " &RHS) {\n";
+ } else {
+ OS << " auto Idx = std::lower_bound(Table.begin(), Table.end(), Key,\n";
+ OS << " [](const " << IndexTypeName << " &LHS, const KeyType &RHS) {\n";
+ }
for (const auto &Field : Index.Fields) {
if (isa<StringRecTy>(Field.RecType)) {
@@ -478,25 +500,34 @@ void SearchableTableEmitter::emitLookupFunction(const GenericTable &Table,
OS << " return false;\n";
OS << " });\n\n";
- OS << " if (Idx == Table.end()";
-
- for (const auto &Field : Index.Fields)
- OS << " ||\n Key." << Field.Name << " != Idx->" << Field.Name;
- OS << ")\n return nullptr;\n";
+ if (!ShouldReturnRange) {
+ OS << " if (Idx == Table.end()";
+ for (const auto &Field : Index.Fields)
+ OS << " ||\n Key." << Field.Name << " != Idx->" << Field.Name;
+ }
- if (IsPrimary)
+ if (ShouldReturnRange)
+ OS << " return llvm::make_range(It.first, It.second);\n";
+ else if (IsPrimary) {
+ OS << ")\n return nullptr;\n\n";
OS << " return &*Idx;\n";
- else
+ } else {
+ OS << ")\n return nullptr;\n\n";
OS << " return &" << Table.Name << "[Idx->_index];\n";
+ }
OS << "}\n";
}
void SearchableTableEmitter::emitLookupDeclaration(const GenericTable &Table,
const SearchIndex &Index,
+ bool ShouldReturnRange,
raw_ostream &OS) {
- OS << "const " << Table.CppTypeName << " *" << Index.Name << "(";
-
+ if (ShouldReturnRange)
+ OS << "llvm::iterator_range<const " << Table.CppTypeName << " *>";
+ else
+ OS << "const " << Table.CppTypeName << " *";
+ OS << Index.Name << "(";
ListSeparator LS;
for (const auto &Field : Index.Fields)
OS << LS << searchableFieldType(Table, Index, Field, TypeInArgument) << " "
@@ -510,11 +541,12 @@ void SearchableTableEmitter::emitGenericTable(const GenericTable &Table,
// Emit the declarations for the functions that will perform lookup.
if (Table.PrimaryKey) {
- emitLookupDeclaration(Table, *Table.PrimaryKey, OS);
+ auto &Index = Table.PrimaryKey;
+ emitLookupDeclaration(Table, *Index, Index->ReturnRange, OS);
OS << ";\n";
}
for (const auto &Index : Table.Indices) {
- emitLookupDeclaration(Table, *Index, OS);
+ emitLookupDeclaration(Table, *Index, Index->ReturnRange, OS);
OS << ";\n";
}
@@ -540,10 +572,14 @@ void SearchableTableEmitter::emitGenericTable(const GenericTable &Table,
// Indexes are sorted "{ Thing, PrimaryIdx }" arrays, so that a binary
// search can be performed by "Thing".
- if (Table.PrimaryKey)
- emitLookupFunction(Table, *Table.PrimaryKey, true, OS);
+ if (Table.PrimaryKey) {
+ auto &Index = Table.PrimaryKey;
+ emitLookupFunction(Table, *Index, /*IsPrimary=*/true, Index->ReturnRange,
+ OS);
+ }
for (const auto &Index : Table.Indices)
- emitLookupFunction(Table, *Index, false, OS);
+ emitLookupFunction(Table, *Index, /*IsPrimary=*/false, Index->ReturnRange,
+ OS);
OS << "#endif\n\n";
}
@@ -569,11 +605,12 @@ bool SearchableTableEmitter::parseFieldType(GenericField &Field, Init *TypeOf) {
std::unique_ptr<SearchIndex> SearchableTableEmitter::parseSearchIndex(
GenericTable &Table, const RecordVal *KeyRecVal, StringRef Name,
- const std::vector<StringRef> &Key, bool EarlyOut) {
+ const std::vector<StringRef> &Key, bool EarlyOut, bool ReturnRange) {
auto Index = std::make_unique<SearchIndex>();
Index->Name = std::string(Name);
Index->Loc = KeyRecVal->getLoc();
Index->EarlyOut = EarlyOut;
+ Index->ReturnRange = ReturnRange;
for (const auto &FieldName : Key) {
const GenericField *Field = Table.getFieldByName(FieldName);
@@ -769,7 +806,8 @@ void SearchableTableEmitter::run(raw_ostream &OS) {
parseSearchIndex(*Table, TableRec->getValue("PrimaryKey"),
TableRec->getValueAsString("PrimaryKeyName"),
TableRec->getValueAsListOfStrings("PrimaryKey"),
- TableRec->getValueAsBit("PrimaryKeyEarlyOut"));
+ TableRec->getValueAsBit("PrimaryKeyEarlyOut"),
+ TableRec->getValueAsBit("PrimaryKeyReturnRange"));
llvm::stable_sort(Table->Entries, [&](Record *LHS, Record *RHS) {
return compareBy(LHS, RHS, *Table->PrimaryKey);
@@ -793,7 +831,8 @@ void SearchableTableEmitter::run(raw_ostream &OS) {
Table.Indices.push_back(
parseSearchIndex(Table, IndexRec->getValue("Key"), IndexRec->getName(),
IndexRec->getValueAsListOfStrings("Key"),
- IndexRec->getValueAsBit("EarlyOut")));
+ IndexRec->getValueAsBit("EarlyOut"),
+ IndexRec->getValueAsBit("ReturnRange")));
}
// Translate legacy tables.
@@ -848,7 +887,7 @@ void SearchableTableEmitter::run(raw_ostream &OS) {
std::string Name =
(Twine("lookup") + Table->CppTypeName + "By" + Field).str();
Table->Indices.push_back(parseSearchIndex(*Table, Class->getValue(Field),
- Name, {Field}, false));
+ Name, {Field}, false, false));
}
Tables.emplace_back(std::move(Table));
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
quic-garvgupt
force-pushed
the
ReturnRange
branch
from
75c3da2 to
c184a1b
Compare
|
Also: s/encodng/encoding/ in the subject, "for RISCV target" is redundant given the "[RISCV]" prefix and it should mention CSRs somewhere. As for the body, s/frameowrk/framework/, and "for RISCV target" needs a "the". |
jrtc27
reviewed
Jul 1, 2024
| auto Range = RISCVSysReg::lookupSysRegByEncoding(Imm); | ||
| // Accept an immediate representing a named Sys Reg if it satisfies the | ||
| // the requried features. | ||
| for (auto It : Range) { |
There was a problem hiding this comment.
Do these want to be const auto &?
quic-garvgupt
force-pushed
the
ReturnRange
branch
from
c184a1b to
f41a529
Compare
quic-garvgupt
changed the title
quic-garvgupt
force-pushed
the
ReturnRange
branch
from
f41a529 to
f6acefc
Compare
topperc
reviewed
Jul 1, 2024
| else | ||
| markup(O, Markup::Register) << formatImm(Imm); | ||
| auto Range = RISCVSysReg::lookupSysRegByEncoding(Imm); | ||
| for (auto &It : Range) { |
There was a problem hiding this comment.
It is a bad name, it isn't an iterator. It's a reference to a SysReg object.
topperc
reviewed
Jul 1, 2024
| RISCVOperand::createSysReg(SysReg ? SysReg->Name : "", S, Imm)); | ||
| auto Range = RISCVSysReg::lookupSysRegByEncoding(Imm); | ||
| // Accept an immediate representing a named Sys Reg if it satisfies the | ||
| // the requried features. |
quic-garvgupt
force-pushed
the
ReturnRange
branch
from
f6acefc to
58d3932
Compare
…cific CSRs This PR is a follow-up of PR llvm#96174 which added the framework to resolve encoding conflicts among vendor specific CSRs. This PR explicitly enables this only for the RISCV target.
quic-garvgupt
force-pushed
the
ReturnRange
branch
from
c2a559d to
2ab927f
Compare
|
@topperc, can we merge this PR as well? This depends on PR#96174 which is already merged now. |
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
…pecific CSRs (llvm#97287) This PR is a follow-up of PR llvm#96174 which added the framework to resolve encoding conflicts among vendor specific CSRs. This PR explicitly enables this only for the RISCV target.
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.
This PR is a follow-up of PR #96174 which added the framework to resolve
encoding conflicts among vendor specific CSRs. This PR explicitly
enables this only for the RISCV target.