[llvm-tblgen] Fixed 64-bit filters being sliced to 32 bits in FixedLenDecoderEmitter

[cameron314]

Summary

When using the FixedLenDecoderEmitter, llvm-tblgen emits tables with (OPC_ExtractField, OPC_ExtractFilterValue) opcode sequences to match the contiguous fixed bits of a given instruction's encoding. This encoding is represented in a 64-bit integer. However, the filter values were represented in a 32-bit integer. As such, instructions with fixed 64-bit encodings (a rare case, but one which occurs in our backend) resulted in a table with an OPC_ExtractField for all 64 bits, followed by an OPC_ExtractFilterValue containing just the low 32 bits of their encoding, causing the filter never to match.

The exact point at which the slicing occurred is line 631.

[Paul-C-Anagnostopoulos]

Does it make sense to define a const for the value (uint64_t)-1?

Possibly dumb question: There are places where the opcode (Opc) is declared as unsigned. Is that okay?

[cameron314]

I can add a constant.

As far as I understand it, the various unsigned Opc variables actually refer to an opcode ID (in NumberedEncodings) and not the instruction encoding itself, so they should be OK.

[cameron314]

Ping? :-)

[Paul-C-Anagnostopoulos]

LGTM.

llvm/utils/TableGen/FixedLenDecoderEmitter.cpp

static BitsInit &getBitsField(const Record &def, StringRef str) {
   return *bits;
 }
 
 // Representation of the instruction to work on.
 typedef std::vector<bit_value_t> insn_t;
 
 namespace {
 
+static const uint64_t NO_FIXED_SEGMENTS_SENTINEL = -1ULL;
+
 class FilterChooser;
 
 /// Filter - Filter works with FilterChooser to produce the decoding tree for
 /// the ISA.
 ///
 /// It is useful to think of a Filter as governing the switch stmts of the
 /// decoding tree in a certain level.  Each case stmt delegates to an inferior
 /// FilterChooser to decide what further decoding logic to employ, or in another
protected:
   // Map of well-known segment value to the set of uid's with that value.
   std::map<uint64_t, std::vector<EncodingIDAndOpcode>>
       FilteredInstructions;
 
   // Set of uid's with non-constant segment values.
   std::vector<EncodingIDAndOpcode> VariableInstructions;
 
   // Map of well-known segment value to its delegate.
-  std::map<unsigned, std::unique_ptr<const FilterChooser>> FilterChooserMap;
+  std::map<uint64_t, std::unique_ptr<const FilterChooser>> FilterChooserMap;
 
   // Number of instructions which fall under FilteredInstructions category.
   unsigned NumFiltered;
 
   // Keeps track of the last opcode in the filtered bucket.
   EncodingIDAndOpcode LastOpcFiltered;
 
 public:
EncodingIDAndOpcode getSingletonOpc() const {
     return LastOpcFiltered;
   }
 
   // Return the filter chooser for the group of instructions without constant
   // segment values.
   const FilterChooser &getVariableFC() const {
     assert(NumFiltered == 1);
     assert(FilterChooserMap.size() == 1);
-    return *(FilterChooserMap.find((unsigned)-1)->second);
+    return *(FilterChooserMap.find(NO_FIXED_SEGMENTS_SENTINEL)->second);
   }
 
   // Divides the decoding task into sub tasks and delegates them to the
   // inferior FilterChooser's.
   //
   // A special case arises when there's only one entry in the filtered
   // instructions.  In order to unambiguously decode the singleton, we need to
   // match the remaining undecoded encoding bits against the singleton.
void Filter::recurse() {
 
   if (!VariableInstructions.empty()) {
     // Conservatively marks each segment position as BIT_UNSET.
     for (unsigned bitIndex = 0; bitIndex < NumBits; ++bitIndex)
       BitValueArray[StartBit + bitIndex] = BIT_UNSET;
 
     // Delegates to an inferior filter chooser for further processing on this
     // group of instructions whose segment values are variable.
-    FilterChooserMap.insert(
-        std::make_pair(-1U, std::make_unique<FilterChooser>(
-                                Owner->AllInstructions, VariableInstructions,
-                                Owner->Operands, BitValueArray, *Owner)));
+    FilterChooserMap.insert(std::make_pair(NO_FIXED_SEGMENTS_SENTINEL,
+        std::make_unique<FilterChooser>(Owner->AllInstructions,
+            VariableInstructions, Owner->Operands, BitValueArray, *Owner)));
   }
 
   // No need to recurse for a singleton filtered instruction.
   // See also Filter::emit*().
   if (getNumFiltered() == 1) {
     assert(FilterChooserMap.size() == 1);
     return;
   }
void Filter::emitTableEntry(DecoderTableInfo &TableInfo) const {
 
   DecoderTable &Table = TableInfo.Table;
 
   size_t PrevFilter = 0;
   bool HasFallthrough = false;
   for (auto &Filter : FilterChooserMap) {
     // Field value -1 implies a non-empty set of variable instructions.
     // See also recurse().
-    if (Filter.first == (unsigned)-1) {
+    if (Filter.first == NO_FIXED_SEGMENTS_SENTINEL) {
       HasFallthrough = true;
 
       // Each scope should always have at least one filter value to check
       // for.
       assert(PrevFilter != 0 && "empty filter set!");
       FixupList &CurScope = TableInfo.FixupStack.back();
       // Resolve any NumToSkip fixups in the current scope.
       resolveTableFixups(Table, CurScope, Table.size());