[PATCH] D33031: [GlobalISel][TableGen] Fix handling of default operands

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1519
-  for (unsigned I = 0; I < DstINumUses &&
-                       DstINumUses > Dst->getNumChildren() + NumDefaultOperands;
-       ++I) {
----------------
rovka wrote:
> Hi Daniel,
> 
> Did you have some case in mind when counting things this way? It seems that for ARM instructions the predicate operand (which expands to 2 values) should be counted as only 1 operand. I tried to concoct other complicated cases with default operands but my TableGen-fu isn't strong enough and I couldn't find anything relevant in the existing instruction descriptions for any target (but I may have been looking at it the wrong way).
> 
> I ran TableGen with/without this patch for a lot of targets (including some that don't support GlobalISel yet) and the only one where I see any difference in the generated code or in the -warn-on-skipped-patterns -debug-only=gisel-emitter is ARM.
> 
> Please let me know if I'm missing something.
Hi Diana,

I didn't have a particular case in mind when doing it this way. My expectation was that we needed to determine whether the MachineInstr had a complete set of MachineOperands and fill in any that are missing from the default operands. Therefore we needed to count how many default MachineOperands we added to the MachineInstr.

Do you have a particular test case I could look at to see the difference in behaviour?

Looking at this code again, I'm also wondering if there might be a bug involving ComplexPatterns with multiple operands since that's a situation where Dst->getNumChildren() is not equal to the number of MachineOperands added to the instruction.


https://reviews.llvm.org/D33031