[PATCH] D35998: [TableGen] AsmMatcher: fix OpIdx computation when HasOptionalOperands is true

Alexandru Guduleasa via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 28 08:31:34 PDT 2017

alexandru.guduleasa added inline comments.

Comment at: utils/TableGen/AsmMatcherEmitter.cpp:2001
                 << "->" << Op.Class->RenderMethod << "(Inst, "
                 << OpInfo.MINumOperands << ");\n"
                 << "      } else {\n"
niravd wrote:
> It looks like you're only using the final DefaultOffset value in OpIdx calculation and NumDefault isn't used elsewhere so it seems like we don't need the array at all. 
> Does replacing the ++NumDefaults line with
> if (OptionalOperandsMask[i]) ++NumDefaults;
> work?
The array is used because the order in which the operands are added in the MCInst is provided by the "ConversionTable".

For the following instruction, I've added the entry from the "ConversionTable"

Instruction asm str:
"inst$flg1$flg2 rd, rs, Imm"

 // Convert__Reg1_2__Flg1_0__Flg2_1__Reg1_3__s32Imm1_4
{ CVT_95_Reg, 3,
  CVT_95_addFlagOperands_LT_CplOperand_COLON__COLON_k_95_Flg1, 1,
  CVT_95_addFlagOperands_LT_CplOperand_COLON__COLON_k_95_Flg2, 2,
  CVT_95_Reg, 4,
  CVT_95_addImmOperands,5, CVT_Done

The first operand that is added is a register (rd, at position 3 in the Operands vector) since the destination must come first.

The array is computed before we analyze the operands and it provides us with the "correction offset".
We could technically remove the array, but then we would need to recompute the offset before each operand (because they are out of order).


More information about the llvm-commits mailing list