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

[Nirav Dave via Phabricator via llvm-commits]

niravd accepted this revision.
niravd added inline comments.
This revision is now accepted and ready to land.


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:2001
                 << "->" << Op.Class->RenderMethod << "(Inst, "
                 << OpInfo.MINumOperands << ");\n"
                 << "      } else {\n"
----------------
alexandru.guduleasa wrote:
> 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"
> ```
> 
> ```
> ConversionTable:
>  // 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).
> 
Ah. So the reason ++NumDefaults worked before is that it only handled at the end. 

Okay. I don't think there's much else we can do here beyond precomputing all DefaultOffsets per instruction at compile time and adding it to the table which doesn't seem worthwhile. 

Can you add a comment with a little explanation here. and otherwise LGTM.


https://reviews.llvm.org/D35998