From 6cbdf3748e0ae45011374a9c5adac5b10a0812cb Mon Sep 17 00:00:00 2001 From: Rahul Joshi Date: Wed, 9 Jul 2025 16:21:10 -0700 Subject: [PATCH 1/5] [NFC][TableGen] Minor code cleanup in SearchableTableEmitter - Add braces around if/else bodies per LLVM coding standards. - Use range for loops. - use auto for variables initialized with `dyn_cast`. --- .../utils/TableGen/SearchableTableEmitter.cpp | 48 +++++++++---------- 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/llvm/utils/TableGen/SearchableTableEmitter.cpp b/llvm/utils/TableGen/SearchableTableEmitter.cpp index 7d57297eb7c0b..5ae7791f17253 100644 --- a/llvm/utils/TableGen/SearchableTableEmitter.cpp +++ b/llvm/utils/TableGen/SearchableTableEmitter.cpp @@ -32,9 +32,9 @@ using namespace llvm; #define DEBUG_TYPE "searchable-table-emitter" static int64_t getAsInt(const Init *B) { - if (const BitsInit *BI = dyn_cast(B)) + if (const auto *BI = dyn_cast(B)) return *BI->convertInitializerToInt(); - if (const IntInit *II = dyn_cast(B)) + if (const auto *II = dyn_cast(B)) return II->getValue(); llvm_unreachable("Unexpected initializer"); } @@ -126,20 +126,20 @@ class SearchableTableEmitter { std::string primaryRepresentation(SMLoc Loc, const GenericField &Field, const Init *I) { - if (const StringInit *SI = dyn_cast(I)) { + if (const auto *SI = dyn_cast(I)) { if (Field.IsCode || SI->hasCodeFormat()) return SI->getValue().str(); else return SI->getAsString(); - } else if (const BitsInit *BI = dyn_cast(I)) + } else if (const auto *BI = dyn_cast(I)) { return "0x" + utohexstr(getAsInt(BI)); - else if (const BitInit *BI = dyn_cast(I)) + } else if (const auto *BI = dyn_cast(I)) { return BI->getValue() ? "true" : "false"; - else if (Field.IsIntrinsic) + } else if (Field.IsIntrinsic) { return "Intrinsic::" + getIntrinsic(I).EnumName.str(); - else if (Field.IsInstruction) + } else if (Field.IsInstruction) { return I->getAsString(); - else if (Field.Enum) { + if (Field.Enum) { const GenericEnum::Entry *Entry = Field.Enum->getEntry(cast(I)->getDef()); if (!Entry) @@ -152,7 +152,7 @@ class SearchableTableEmitter { } bool isIntrinsic(const Init *I) { - if (const DefInit *DI = dyn_cast(I)) + if (const auto *DI = dyn_cast(I)) return DI->getDef()->isSubClassOf("Intrinsic"); return false; } @@ -174,7 +174,7 @@ class SearchableTableEmitter { if (Ctx == TypeInTempStruct) return "std::string"; return "StringRef"; - } else if (const BitsRecTy *BI = dyn_cast(Field.RecType)) { + } else if (const auto *BI = dyn_cast(Field.RecType)) { unsigned NumBits = BI->getNumBits(); if (NumBits <= 8) return "uint8_t"; @@ -190,8 +190,9 @@ class SearchableTableEmitter { "' of type bits is too large"); } else if (isa(Field.RecType)) { return "bool"; - } else if (Field.Enum || Field.IsIntrinsic || Field.IsInstruction) + } else if (Field.Enum || Field.IsIntrinsic || Field.IsInstruction) { return "unsigned"; + } PrintFatalError(Index.Loc, Twine("In table '") + Table.Name + "' lookup method '" + Index.Name + "', key field '" + Field.Name + @@ -359,8 +360,8 @@ void SearchableTableEmitter::emitLookupFunction(const GenericTable &Table, std::vector> Entries; Entries.reserve(Table.Entries.size()); - for (unsigned i = 0; i < Table.Entries.size(); ++i) - Entries.emplace_back(Table.Entries[i], i); + for (auto [Idx, TblEntry] : enumerate(Table.Entries)) + Entries.emplace_back(TblEntry, Idx); llvm::stable_sort(Entries, [&](const std::pair &LHS, @@ -369,19 +370,19 @@ void SearchableTableEmitter::emitLookupFunction(const GenericTable &Table, }); IndexRowsStorage.reserve(Entries.size()); - for (const auto &Entry : Entries) { - IndexRowsStorage.push_back(Entry.first); + for (const auto &[EntryRec, EntryIndex] : Entries) { + IndexRowsStorage.push_back(EntryRec); OS << " { "; ListSeparator LS; for (const auto &Field : Index.Fields) { std::string Repr = primaryRepresentation( - Index.Loc, Field, Entry.first->getValueInit(Field.Name)); + Index.Loc, Field, EntryRec->getValueInit(Field.Name)); if (isa(Field.RecType)) Repr = StringRef(Repr).upper(); OS << LS << Repr; } - OS << ", " << Entry.second << " },\n"; + OS << ", " << EntryIndex << " },\n"; } OS << " };\n\n"; @@ -398,8 +399,8 @@ void SearchableTableEmitter::emitLookupFunction(const GenericTable &Table, Index.Fields[0].IsInstruction)) { int64_t FirstKeyVal = getNumericKey(Index, IndexRows[0]); IsContiguous = true; - for (unsigned i = 0; i < IndexRows.size(); ++i) { - if (getNumericKey(Index, IndexRows[i]) != (FirstKeyVal + i)) { + for (const auto &[Idx, IndexRow] : enumerate(IndexRows)) { + if (getNumericKey(Index, IndexRow) != FirstKeyVal + (int64_t)Idx) { IsContiguous = false; break; } @@ -509,9 +510,9 @@ void SearchableTableEmitter::emitLookupFunction(const GenericTable &Table, OS << " ||\n Key." << Field.Name << " != Idx->" << Field.Name; } - if (ShouldReturnRange) + if (ShouldReturnRange) { OS << " return llvm::make_range(It.first, It.second);\n"; - else if (IsPrimary) { + } else if (IsPrimary) { OS << ")\n return nullptr;\n\n"; OS << " return &*Idx;\n"; } else { @@ -557,8 +558,7 @@ void SearchableTableEmitter::emitGenericTable(const GenericTable &Table, // The primary data table contains all the fields defined for this map. OS << "constexpr " << Table.CppTypeName << " " << Table.Name << "[] = {\n"; - for (unsigned i = 0; i < Table.Entries.size(); ++i) { - const Record *Entry = Table.Entries[i]; + for (const auto &[Idx, Entry] : enumerate(Table.Entries)) { OS << " { "; ListSeparator LS; @@ -567,7 +567,7 @@ void SearchableTableEmitter::emitGenericTable(const GenericTable &Table, << primaryRepresentation(Table.Locs[0], Field, Entry->getValueInit(Field.Name)); - OS << " }, // " << i << "\n"; + OS << " }, // " << Idx << "\n"; } OS << " };\n"; From 0a4f6aed0f7a45a66736bbac92cb896b7df10237 Mon Sep 17 00:00:00 2001 From: Rahul Joshi Date: Wed, 9 Jul 2025 22:14:28 -0700 Subject: [PATCH 2/5] Review feedback --- .../utils/TableGen/SearchableTableEmitter.cpp | 122 ++++++++++-------- 1 file changed, 65 insertions(+), 57 deletions(-) diff --git a/llvm/utils/TableGen/SearchableTableEmitter.cpp b/llvm/utils/TableGen/SearchableTableEmitter.cpp index 5ae7791f17253..3b49ca7839b55 100644 --- a/llvm/utils/TableGen/SearchableTableEmitter.cpp +++ b/llvm/utils/TableGen/SearchableTableEmitter.cpp @@ -131,17 +131,17 @@ class SearchableTableEmitter { return SI->getValue().str(); else return SI->getAsString(); - } else if (const auto *BI = dyn_cast(I)) { + } + if (const auto *BI = dyn_cast(I)) return "0x" + utohexstr(getAsInt(BI)); - } else if (const auto *BI = dyn_cast(I)) { + if (const auto *BI = dyn_cast(I)) return BI->getValue() ? "true" : "false"; - } else if (Field.IsIntrinsic) { + if (Field.IsIntrinsic) return "Intrinsic::" + getIntrinsic(I).EnumName.str(); - } else if (Field.IsInstruction) { + if (Field.IsInstruction) return I->getAsString(); if (Field.Enum) { - const GenericEnum::Entry *Entry = - Field.Enum->getEntry(cast(I)->getDef()); + auto *Entry = Field.Enum->EntryMap[cast(I)->getDef()]; if (!Entry) PrintFatalError(Loc, Twine("Entry for field '") + Field.Name + "' is null"); @@ -174,7 +174,8 @@ class SearchableTableEmitter { if (Ctx == TypeInTempStruct) return "std::string"; return "StringRef"; - } else if (const auto *BI = dyn_cast(Field.RecType)) { + } + if (const auto *BI = dyn_cast(Field.RecType)) { unsigned NumBits = BI->getNumBits(); if (NumBits <= 8) return "uint8_t"; @@ -188,11 +189,11 @@ class SearchableTableEmitter { "' lookup method '" + Index.Name + "', key field '" + Field.Name + "' of type bits is too large"); - } else if (isa(Field.RecType)) { + } + if (isa(Field.RecType)) return "bool"; - } else if (Field.Enum || Field.IsIntrinsic || Field.IsInstruction) { + if (Field.Enum || Field.IsIntrinsic || Field.IsInstruction) return "unsigned"; - } PrintFatalError(Index.Loc, Twine("In table '") + Table.Name + "' lookup method '" + Index.Name + "', key field '" + Field.Name + @@ -245,67 +246,74 @@ int64_t SearchableTableEmitter::getNumericKey(const SearchIndex &Index, /// key of \p Index. bool SearchableTableEmitter::compareBy(const Record *LHS, const Record *RHS, const SearchIndex &Index) { - for (const auto &Field : Index.Fields) { - const Init *LHSI = LHS->getValueInit(Field.Name); - const Init *RHSI = RHS->getValueInit(Field.Name); + // Compare two values and return: + // true if LHS < RHS + // false if LHS > RHS + // std::nullopt if LHS == RHS + auto CmpLTValue = [](const auto &LHS, + const auto &RHS) -> std::optional { + if (LHS < RHS) + return true; + if (LHS > RHS) + return false; + return std::nullopt; + }; + // Compare two fields and returns: + // true if LHS < RHS + // false if LHS > RHS + // std::nullopt if LHS == RHS + auto CmpLTField = [this, &Index, CmpLTValue]( + const Init *LHSI, const Init *RHSI, + const GenericField &Field) -> std::optional { if (isa(Field.RecType) || isa(Field.RecType)) { int64_t LHSi = getAsInt(LHSI); int64_t RHSi = getAsInt(RHSI); - if (LHSi < RHSi) - return true; - if (LHSi > RHSi) - return false; - } else if (Field.IsIntrinsic) { + return CmpLTValue(LHSi, RHSi); + } + + if (Field.IsIntrinsic) { const CodeGenIntrinsic &LHSi = getIntrinsic(LHSI); const CodeGenIntrinsic &RHSi = getIntrinsic(RHSI); - if (std::tie(LHSi.TargetPrefix, LHSi.Name) < - std::tie(RHSi.TargetPrefix, RHSi.Name)) - return true; - if (std::tie(LHSi.TargetPrefix, LHSi.Name) > - std::tie(RHSi.TargetPrefix, RHSi.Name)) - return false; - } else if (Field.IsInstruction) { + return CmpLTValue(std::tie(LHSi.TargetPrefix, LHSi.Name), + std::tie(RHSi.TargetPrefix, RHSi.Name)); + } + + if (Field.IsInstruction) { // This does not correctly compare the predefined instructions! const Record *LHSr = cast(LHSI)->getDef(); const Record *RHSr = cast(RHSI)->getDef(); - bool LHSpseudo = LHSr->getValueAsBit("isPseudo"); - bool RHSpseudo = RHSr->getValueAsBit("isPseudo"); - if (LHSpseudo && !RHSpseudo) - return true; - if (!LHSpseudo && RHSpseudo) - return false; + // Order pseudo instructions before non-pseudo ones. + bool LHSNotPseudo = !LHSr->getValueAsBit("isPseudo"); + bool RHSNotPseudo = !RHSr->getValueAsBit("isPseudo"); + return CmpLTValue(std::tuple(LHSNotPseudo, LHSr->getName()), + std::tuple(RHSNotPseudo, RHSr->getName())); + } - int comp = LHSr->getName().compare(RHSr->getName()); - if (comp < 0) - return true; - if (comp > 0) - return false; - } else if (Field.Enum) { - auto LHSr = cast(LHSI)->getDef(); - auto RHSr = cast(RHSI)->getDef(); - int64_t LHSv = Field.Enum->getEntry(LHSr)->Value; - int64_t RHSv = Field.Enum->getEntry(RHSr)->Value; - if (LHSv < RHSv) - return true; - if (LHSv > RHSv) - return false; - } else { - std::string LHSs = primaryRepresentation(Index.Loc, Field, LHSI); - std::string RHSs = primaryRepresentation(Index.Loc, Field, RHSI); + if (Field.Enum) { + const Record *LHSr = cast(LHSI)->getDef(); + const Record *RHSr = cast(RHSI)->getDef(); + int64_t LHSv = Field.Enum->EntryMap[LHSr]->second; + int64_t RHSv = Field.Enum->EntryMap[RHSr]->second; + return CmpLTValue(LHSv, RHSv); + } - if (isa(Field.RecType)) { - LHSs = StringRef(LHSs).upper(); - RHSs = StringRef(RHSs).upper(); - } + std::string LHSs = primaryRepresentation(Index.Loc, Field, LHSI); + std::string RHSs = primaryRepresentation(Index.Loc, Field, RHSI); - int comp = LHSs.compare(RHSs); - if (comp < 0) - return true; - if (comp > 0) - return false; + if (isa(Field.RecType)) { + LHSs = StringRef(LHSs).upper(); + RHSs = StringRef(RHSs).upper(); } + return CmpLTValue(LHSs, RHSs); + }; + + for (const GenericField &Field : Index.Fields) { + const Init *LHSI = LHS->getValueInit(Field.Name); + const Init *RHSI = RHS->getValueInit(Field.Name); + if (std::optional Cmp = CmpLTField(LHSI, RHSI, Field)) + return *Cmp; } return false; } From b9f9093f58b9f570b6b354a52ee219cf2a92785e Mon Sep 17 00:00:00 2001 From: Rahul Joshi Date: Thu, 10 Jul 2025 08:54:22 -0700 Subject: [PATCH 3/5] Use compare() to order strings --- .../utils/TableGen/SearchableTableEmitter.cpp | 38 ++++++++++++------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/llvm/utils/TableGen/SearchableTableEmitter.cpp b/llvm/utils/TableGen/SearchableTableEmitter.cpp index 3b49ca7839b55..71389ceb7b326 100644 --- a/llvm/utils/TableGen/SearchableTableEmitter.cpp +++ b/llvm/utils/TableGen/SearchableTableEmitter.cpp @@ -247,9 +247,9 @@ int64_t SearchableTableEmitter::getNumericKey(const SearchIndex &Index, bool SearchableTableEmitter::compareBy(const Record *LHS, const Record *RHS, const SearchIndex &Index) { // Compare two values and return: - // true if LHS < RHS - // false if LHS > RHS - // std::nullopt if LHS == RHS + // - true if LHS < RHS. + // - false if LHS > RHS. + // - std::nullopt if LHS == RHS. auto CmpLTValue = [](const auto &LHS, const auto &RHS) -> std::optional { if (LHS < RHS) @@ -259,11 +259,21 @@ bool SearchableTableEmitter::compareBy(const Record *LHS, const Record *RHS, return std::nullopt; }; + // Specialized form of `CmpLTValue` for string-like types that uses compare() + // to do the comparison of the 2 strings once (instead if 2 comparisons if we + // use `CmpLTValue`). + auto CmpLTString = [](StringRef LHS, StringRef RHS) -> std::optional { + int Cmp = LHS.compare(RHS); + if (Cmp == 0) + return std::nullopt; + return Cmp < 0; + }; + // Compare two fields and returns: - // true if LHS < RHS - // false if LHS > RHS - // std::nullopt if LHS == RHS - auto CmpLTField = [this, &Index, CmpLTValue]( + // - true if LHS < RHS. + // - false if LHS > RHS. + // - std::nullopt if LHS == RHS. + auto CmpLTField = [this, &Index, &CmpLTValue, &CmpLTString]( const Init *LHSI, const Init *RHSI, const GenericField &Field) -> std::optional { if (isa(Field.RecType) || isa(Field.RecType)) { @@ -275,8 +285,10 @@ bool SearchableTableEmitter::compareBy(const Record *LHS, const Record *RHS, if (Field.IsIntrinsic) { const CodeGenIntrinsic &LHSi = getIntrinsic(LHSI); const CodeGenIntrinsic &RHSi = getIntrinsic(RHSI); - return CmpLTValue(std::tie(LHSi.TargetPrefix, LHSi.Name), - std::tie(RHSi.TargetPrefix, RHSi.Name)); + if (std::optional Cmp = + CmpLTString(LHSi.TargetPrefix, RHSi.TargetPrefix)) + return *Cmp; + return CmpLTString(LHSi.Name, RHSi.Name); } if (Field.IsInstruction) { @@ -287,8 +299,9 @@ bool SearchableTableEmitter::compareBy(const Record *LHS, const Record *RHS, // Order pseudo instructions before non-pseudo ones. bool LHSNotPseudo = !LHSr->getValueAsBit("isPseudo"); bool RHSNotPseudo = !RHSr->getValueAsBit("isPseudo"); - return CmpLTValue(std::tuple(LHSNotPseudo, LHSr->getName()), - std::tuple(RHSNotPseudo, RHSr->getName())); + if (std::optional Cmp = CmpLTValue(LHSNotPseudo, RHSNotPseudo)) + return *Cmp; + return CmpLTString(LHSr->getName(), RHSr->getName()); } if (Field.Enum) { @@ -301,12 +314,11 @@ bool SearchableTableEmitter::compareBy(const Record *LHS, const Record *RHS, std::string LHSs = primaryRepresentation(Index.Loc, Field, LHSI); std::string RHSs = primaryRepresentation(Index.Loc, Field, RHSI); - if (isa(Field.RecType)) { LHSs = StringRef(LHSs).upper(); RHSs = StringRef(RHSs).upper(); } - return CmpLTValue(LHSs, RHSs); + return CmpLTString(LHSs, RHSs); }; for (const GenericField &Field : Index.Fields) { From 2390e7f9d75a873af176bc9b5a3c5ad347aa17eb Mon Sep 17 00:00:00 2001 From: Rahul Joshi Date: Thu, 10 Jul 2025 09:02:52 -0700 Subject: [PATCH 4/5] Use -1/0/1 instead of optional for comparison result --- .../utils/TableGen/SearchableTableEmitter.cpp | 41 ++++++++----------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/llvm/utils/TableGen/SearchableTableEmitter.cpp b/llvm/utils/TableGen/SearchableTableEmitter.cpp index 71389ceb7b326..106322dec552c 100644 --- a/llvm/utils/TableGen/SearchableTableEmitter.cpp +++ b/llvm/utils/TableGen/SearchableTableEmitter.cpp @@ -247,35 +247,31 @@ int64_t SearchableTableEmitter::getNumericKey(const SearchIndex &Index, bool SearchableTableEmitter::compareBy(const Record *LHS, const Record *RHS, const SearchIndex &Index) { // Compare two values and return: - // - true if LHS < RHS. - // - false if LHS > RHS. - // - std::nullopt if LHS == RHS. - auto CmpLTValue = [](const auto &LHS, - const auto &RHS) -> std::optional { + // * -1 if LHS < RHS. + // * 1 if LHS > RHS. + // * 0 if LHS == RHS. + auto CmpLTValue = [](const auto &LHS, const auto &RHS) -> int { if (LHS < RHS) - return true; + return -1; if (LHS > RHS) - return false; - return std::nullopt; + return 1; + return 0; }; // Specialized form of `CmpLTValue` for string-like types that uses compare() // to do the comparison of the 2 strings once (instead if 2 comparisons if we // use `CmpLTValue`). - auto CmpLTString = [](StringRef LHS, StringRef RHS) -> std::optional { - int Cmp = LHS.compare(RHS); - if (Cmp == 0) - return std::nullopt; - return Cmp < 0; + auto CmpLTString = [](StringRef LHS, StringRef RHS) -> int { + return LHS.compare(RHS); }; // Compare two fields and returns: // - true if LHS < RHS. // - false if LHS > RHS. // - std::nullopt if LHS == RHS. - auto CmpLTField = [this, &Index, &CmpLTValue, &CmpLTString]( - const Init *LHSI, const Init *RHSI, - const GenericField &Field) -> std::optional { + auto CmpLTField = [this, &Index, &CmpLTValue, + &CmpLTString](const Init *LHSI, const Init *RHSI, + const GenericField &Field) -> int { if (isa(Field.RecType) || isa(Field.RecType)) { int64_t LHSi = getAsInt(LHSI); int64_t RHSi = getAsInt(RHSI); @@ -285,9 +281,8 @@ bool SearchableTableEmitter::compareBy(const Record *LHS, const Record *RHS, if (Field.IsIntrinsic) { const CodeGenIntrinsic &LHSi = getIntrinsic(LHSI); const CodeGenIntrinsic &RHSi = getIntrinsic(RHSI); - if (std::optional Cmp = - CmpLTString(LHSi.TargetPrefix, RHSi.TargetPrefix)) - return *Cmp; + if (int Cmp = CmpLTString(LHSi.TargetPrefix, RHSi.TargetPrefix)) + return Cmp; return CmpLTString(LHSi.Name, RHSi.Name); } @@ -299,8 +294,8 @@ bool SearchableTableEmitter::compareBy(const Record *LHS, const Record *RHS, // Order pseudo instructions before non-pseudo ones. bool LHSNotPseudo = !LHSr->getValueAsBit("isPseudo"); bool RHSNotPseudo = !RHSr->getValueAsBit("isPseudo"); - if (std::optional Cmp = CmpLTValue(LHSNotPseudo, RHSNotPseudo)) - return *Cmp; + if (int Cmp = CmpLTValue(LHSNotPseudo, RHSNotPseudo)) + return Cmp; return CmpLTString(LHSr->getName(), RHSr->getName()); } @@ -324,8 +319,8 @@ bool SearchableTableEmitter::compareBy(const Record *LHS, const Record *RHS, for (const GenericField &Field : Index.Fields) { const Init *LHSI = LHS->getValueInit(Field.Name); const Init *RHSI = RHS->getValueInit(Field.Name); - if (std::optional Cmp = CmpLTField(LHSI, RHSI, Field)) - return *Cmp; + if (int Cmp = CmpLTField(LHSI, RHSI, Field)) + return Cmp < 0; } return false; } From 1b8751794effb70314e26546e1c4d987496110fe Mon Sep 17 00:00:00 2001 From: Rahul Joshi Date: Thu, 10 Jul 2025 13:16:27 -0700 Subject: [PATCH 5/5] Resolve conflicts --- llvm/utils/TableGen/SearchableTableEmitter.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/llvm/utils/TableGen/SearchableTableEmitter.cpp b/llvm/utils/TableGen/SearchableTableEmitter.cpp index 106322dec552c..d17d90b452bd7 100644 --- a/llvm/utils/TableGen/SearchableTableEmitter.cpp +++ b/llvm/utils/TableGen/SearchableTableEmitter.cpp @@ -141,7 +141,8 @@ class SearchableTableEmitter { if (Field.IsInstruction) return I->getAsString(); if (Field.Enum) { - auto *Entry = Field.Enum->EntryMap[cast(I)->getDef()]; + const GenericEnum::Entry *Entry = + Field.Enum->getEntry(cast(I)->getDef()); if (!Entry) PrintFatalError(Loc, Twine("Entry for field '") + Field.Name + "' is null"); @@ -302,8 +303,8 @@ bool SearchableTableEmitter::compareBy(const Record *LHS, const Record *RHS, if (Field.Enum) { const Record *LHSr = cast(LHSI)->getDef(); const Record *RHSr = cast(RHSI)->getDef(); - int64_t LHSv = Field.Enum->EntryMap[LHSr]->second; - int64_t RHSv = Field.Enum->EntryMap[RHSr]->second; + int64_t LHSv = Field.Enum->getEntry(LHSr)->Value; + int64_t RHSv = Field.Enum->getEntry(RHSr)->Value; return CmpLTValue(LHSv, RHSv); }