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

Wed May 10 02:16:34 PDT 2017

dsanders added inline comments.

================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1519
-  for (unsigned I = 0; I < DstINumUses &&
-                       DstINumUses > Dst->getNumChildren() + NumDefaultOperands;
-       ++I) {
----------------
dsanders wrote:
> 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.
> Do you have a particular test case I could look at to see the difference in behaviour?

I mean from ARM. In the meantime, I'll look into the one in this patch.

https://reviews.llvm.org/D33031