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

[Alexandru Guduleasa via Phabricator via llvm-commits]

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"
```

```
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).



https://reviews.llvm.org/D35998