From d919d4d28a2d2343abf68d14ea908d514986ebbf Mon Sep 17 00:00:00 2001 From: Jason Eckhardt Date: Mon, 20 Jan 2025 10:35:06 -0600 Subject: [PATCH 1/3] [TableGen][NFC] Factor early-out range check. Combine the EarlyOut and IsContiguous range check. Also avoid "comparison is always false" warnings in emitted code when the lower-bound check is against 0. --- .../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 38b6f2b395137..5788e6eb56a68 100644 --- a/llvm/utils/TableGen/SearchableTableEmitter.cpp +++ b/llvm/utils/TableGen/SearchableTableEmitter.cpp @@ -392,37 +392,32 @@ void SearchableTableEmitter::emitLookupFunction(const GenericTable &Table, } } - if (IsContiguous) { + if (Index.EarlyOut || IsContiguous) { const GenericField &Field = Index.Fields[0]; std::string FirstRepr = primaryRepresentation( Index.Loc, Field, IndexRows[0]->getValueInit(Field.Name)); std::string LastRepr = primaryRepresentation( Index.Loc, Field, IndexRows.back()->getValueInit(Field.Name)); - OS << " if ((" << Field.Name << " < " << FirstRepr << ") ||\n"; - OS << " (" << Field.Name << " > " << LastRepr << "))\n"; - OS << " return nullptr;\n"; - OS << " auto Table = ArrayRef(" << IndexName << ");\n"; - OS << " size_t Idx = " << Index.Fields[0].Name << " - " << FirstRepr - << ";\n"; - OS << " return "; - if (IsPrimary) - OS << "&Table[Idx]"; + if (getNumericKey(Index, IndexRows[0]) == 0) + OS << " if ("; else - OS << "&" << Table.Name << "[Table[Idx]._index]"; - OS << ";\n"; - OS << "}\n"; - return; - } - - if (Index.EarlyOut) { - const GenericField &Field = Index.Fields[0]; - std::string FirstRepr = primaryRepresentation( - Index.Loc, Field, IndexRows[0]->getValueInit(Field.Name)); - std::string LastRepr = primaryRepresentation( - Index.Loc, Field, IndexRows.back()->getValueInit(Field.Name)); - OS << " if ((" << Field.Name << " < " << FirstRepr << ") ||\n"; + OS << " if ((" << Field.Name << " < " << FirstRepr << ") ||\n"; OS << " (" << Field.Name << " > " << LastRepr << "))\n"; OS << " return nullptr;\n\n"; + + if (IsContiguous) { + OS << " auto Table = ArrayRef(" << IndexName << ");\n"; + OS << " size_t Idx = " << Index.Fields[0].Name << " - " << FirstRepr + << ";\n"; + OS << " return "; + if (IsPrimary) + OS << "&Table[Idx]"; + else + OS << "&" << Table.Name << "[Table[Idx]._index]"; + OS << ";\n"; + OS << "}\n"; + return; + } } OS << " struct KeyType {\n"; From 5a5e07274b4227b6089d0f50ef87f1ca5ab59790 Mon Sep 17 00:00:00 2001 From: Jason Eckhardt Date: Mon, 20 Jan 2025 14:38:24 -0600 Subject: [PATCH 2/3] wip --- .../utils/TableGen/SearchableTableEmitter.cpp | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/llvm/utils/TableGen/SearchableTableEmitter.cpp b/llvm/utils/TableGen/SearchableTableEmitter.cpp index 5788e6eb56a68..2ee996bffb381 100644 --- a/llvm/utils/TableGen/SearchableTableEmitter.cpp +++ b/llvm/utils/TableGen/SearchableTableEmitter.cpp @@ -211,20 +211,21 @@ class SearchableTableEmitter { // known, return that numeric value. int64_t SearchableTableEmitter::getNumericKey(const SearchIndex &Index, const Record *Rec) { - assert(Index.Fields.size() == 1); + const GenericField &Field = Index.Fields[0]; // To be consistent with compareBy and primaryRepresentation elsewhere, // we check for IsInstruction before Enum-- these fields are not exclusive. - if (Index.Fields[0].IsInstruction) { - const Record *TheDef = Rec->getValueAsDef(Index.Fields[0].Name); + if (Field.IsInstruction) { + const Record *TheDef = Rec->getValueAsDef(Field.Name); return Target->getInstrIntValue(TheDef); } - if (Index.Fields[0].Enum) { - const Record *EnumEntry = Rec->getValueAsDef(Index.Fields[0].Name); - return Index.Fields[0].Enum->EntryMap[EnumEntry]->second; + if (Field.Enum) { + const Record *EnumEntry = Rec->getValueAsDef(Field.Name); + return Field.Enum->EntryMap[EnumEntry]->second; } + assert(isa(Field.RecType) && "unexpected field type"); - return getInt(Rec, Index.Fields[0].Name); + return getInt(Rec, Field.Name); } /// Less-than style comparison between \p LHS and \p RHS according to the @@ -405,10 +406,9 @@ void SearchableTableEmitter::emitLookupFunction(const GenericTable &Table, OS << " (" << Field.Name << " > " << LastRepr << "))\n"; OS << " return nullptr;\n\n"; - if (IsContiguous) { + if (IsContiguous && !Index.EarlyOut) { OS << " auto Table = ArrayRef(" << IndexName << ");\n"; - OS << " size_t Idx = " << Index.Fields[0].Name << " - " << FirstRepr - << ";\n"; + OS << " size_t Idx = " << Field.Name << " - " << FirstRepr << ";\n"; OS << " return "; if (IsPrimary) OS << "&Table[Idx]"; From c28604c0d54f8bd3dbaffc7c40cf1eb46bea606b Mon Sep 17 00:00:00 2001 From: Jason Eckhardt Date: Mon, 20 Jan 2025 17:33:28 -0600 Subject: [PATCH 3/3] Use clamp, update test accordingly. --- llvm/test/TableGen/generic-tables-instruction.td | 9 +++------ llvm/test/TableGen/generic-tables.td | 3 +-- llvm/utils/TableGen/SearchableTableEmitter.cpp | 11 ++++++----- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/llvm/test/TableGen/generic-tables-instruction.td b/llvm/test/TableGen/generic-tables-instruction.td index 3be2462c9ab69..7b99784f2d73a 100644 --- a/llvm/test/TableGen/generic-tables-instruction.td +++ b/llvm/test/TableGen/generic-tables-instruction.td @@ -18,14 +18,12 @@ def Arch : Target { let InstructionSet = ArchInstrInfo; } // A contiguous primary (Instruction) key should get a direct lookup instead of // binary search. // CHECK: const MyInstr *getCustomEncodingHelper(unsigned Opcode) { -// CHECK: if ((Opcode < B) || -// CHECK: (Opcode > D)) -// CHECK: return nullptr; +// CHECK: if ((unsigned)Opcode != std::clamp((unsigned)Opcode, (unsigned)B, (unsigned)D)) +// CHECK: return nullptr; // CHECK: auto Table = ArrayRef(InstrTable); // CHECK: size_t Idx = Opcode - B; // CHECK: return &Table[Idx]; - class MyInstr : Instruction { let OutOperandList = (outs); let InOperandList = (ins); @@ -56,8 +54,7 @@ def InstrTable : GenericTable { // // Verify contiguous check for SearchIndex. // const MyInfoEntry *getTable2ByValue(uint8_t Value) { -// CHECK: if ((Value < 0xB) || -// CHECK: (Value > 0xD)) +// CHECK: if ((uint8_t)Value != std::clamp((uint8_t)Value, (uint8_t)0xB, (uint8_t)0xD)) // CHECK: return nullptr; // CHECK: auto Table = ArrayRef(Index); // CHECK: size_t Idx = Value - 0xB; diff --git a/llvm/test/TableGen/generic-tables.td b/llvm/test/TableGen/generic-tables.td index 003b532093460..8638740a8e565 100644 --- a/llvm/test/TableGen/generic-tables.td +++ b/llvm/test/TableGen/generic-tables.td @@ -113,8 +113,7 @@ def lookupBTableByNameAndFlag : SearchIndex { // CHECK: const CEntry *lookupCEntry(StringRef Name, unsigned Kind); // CHECK-LABEL: GET_CTable_IMPL // CHECK: const CEntry *lookupCEntryByEncoding(uint16_t Encoding) { -// CHECK: if ((Encoding < 0xA) || -// CHECK: (Encoding > 0xF)) +// CHECK: if ((uint16_t)Encoding != std::clamp((uint16_t)Encoding, (uint16_t)0xA, (uint16_t)0xF)) // CHECK: return nullptr; // CHECK: const CEntry *lookupCEntry(StringRef Name, unsigned Kind) { diff --git a/llvm/utils/TableGen/SearchableTableEmitter.cpp b/llvm/utils/TableGen/SearchableTableEmitter.cpp index 2ee996bffb381..e91baf98e9ffc 100644 --- a/llvm/utils/TableGen/SearchableTableEmitter.cpp +++ b/llvm/utils/TableGen/SearchableTableEmitter.cpp @@ -211,6 +211,7 @@ class SearchableTableEmitter { // known, return that numeric value. int64_t SearchableTableEmitter::getNumericKey(const SearchIndex &Index, const Record *Rec) { + assert(Index.Fields.size() == 1); const GenericField &Field = Index.Fields[0]; // To be consistent with compareBy and primaryRepresentation elsewhere, @@ -399,11 +400,11 @@ void SearchableTableEmitter::emitLookupFunction(const GenericTable &Table, Index.Loc, Field, IndexRows[0]->getValueInit(Field.Name)); std::string LastRepr = primaryRepresentation( Index.Loc, Field, IndexRows.back()->getValueInit(Field.Name)); - if (getNumericKey(Index, IndexRows[0]) == 0) - OS << " if ("; - else - OS << " if ((" << Field.Name << " < " << FirstRepr << ") ||\n"; - OS << " (" << Field.Name << " > " << LastRepr << "))\n"; + std::string TS = + '(' + searchableFieldType(Table, Index, Field, TypeInStaticStruct) + + ')'; + OS << " if (" << TS << Field.Name << " != std::clamp(" << TS << Field.Name + << ", " << TS << FirstRepr << ", " << TS << LastRepr << "))\n"; OS << " return nullptr;\n\n"; if (IsContiguous && !Index.EarlyOut) {