[PATCH] D52100: [tblgen] Allow FixedLenDecoderEmitter to use APInt-like objects as InsnType

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

Apart from the issue with the assertion, this change looks good to me.



================
Comment at: utils/TableGen/FixedLenDecoderEmitter.cpp:2119-2120
      << "                                     unsigned numBits) {\n"
-     << "    assert(startBit + numBits <= (sizeof(InsnType)*8) &&\n"
-     << "           \"Instruction field out of bounds!\");\n"
-     << "    InsnType fieldMask;\n"
-     << "    if (numBits == sizeof(InsnType)*8)\n"
-     << "      fieldMask = (InsnType)(-1LL);\n"
-     << "    else\n"
-     << "      fieldMask = (((InsnType)1 << numBits) - 1) << startBit;\n"
-     << "    return (insn & fieldMask) >> startBit;\n"
+     << "  assert(startBit + numBits <= 64 && \"Cannot support >64-bit "
+        "extractions!\");\n"
+     << "  return fieldFromInstruction(insn, startBit, numBits, "
----------------
dsanders wrote:
> nhaehnle wrote:
> > This assertion seems to defeat the point of the change?
> This assertion is what I was referring to in the summary with 'There is one catch with this at the moment, no string of bits extracted from the encoding may exceed 64-bits'. The assertion is there to catch the case where the generated matcher table falls foul of some remaining implicit casts to uint64_t that I haven't managed to eliminate yet but probably don't need to because it's very rare that ISA's that needed the space afforded by large encodings will hit it.
> 
> Suppose you have an ISA with a single instruction:
>   def A : Instruction { let Inst{0-127} = 0; }
> That will currently fail to work, because the generated matcher will extract 128 bits in a single command:
>   if Inst[0-127] == 0: # We don't support this extraction
>     Instruction is A
> This will also fail for the same reason:
>   def A : Instruction { let Inst{0-127} = 0; }
>   def B : Instruction { let Inst{0-62} = 0; let Inst{63} = 1; let Inst{64-127} = 0; }
> because it's still a 128-bit literal being checked:
>   if Inst[0-127] == 0: # We don't support this extraction
>     Instruction is A
>   if Inst[0-127] == 0b000....010.....000: # We don't support this extraction
>     Instruction is B
> 
> However, this ISA:
>   def A : Instruction { let Inst{0-127} = 0; }
>   def B : Instruction {
>     bits<32> Imm;
>     let Inst{0-62} = Imm;
>     let Inst{63} = 1;
>     let Inst{64-127} = 0;
>   }
> will succeed since the generated matcher will only generate extractions of 64-bits or less:
>   if Inst[63] == 0:
>     if Inst[0-62] == 0 and Inst[64-127] == 0:
>       Instruction is A
>   else:
>     if Inst[64-127] == 0:
>       Instruction is B
>       Imm is Inst[0-63]
> 
> To summarize, targets should only hit this assertion if they haven't implemented enough of their ISA to justify the length of the encodings. In the case of the out of tree target I'm working on, it was only a problem when nop's were the only thing implemented. As soon as I implemented an add instruction the matcher was complicated enough to not hit this corner case.
Extracting Inst[64-127] will use startBit == 64, numBits == 64, right? So that will fail the assertion.


Repository:
  rL LLVM

https://reviews.llvm.org/D52100