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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders accepted this revision.
dsanders added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: utils/TableGen/GlobalISelEmitter.cpp:1519
-  for (unsigned I = 0; I < DstINumUses &&
-                       DstINumUses > Dst->getNumChildren() + NumDefaultOperands;
-       ++I) {
----------------
rovka wrote:
> dsanders wrote:
> > dsanders wrote:
> > > dsanders wrote:
> > > > 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.
> > > The bug originates from DstI.Operands.size() which doesn't have the value I'd expect on XORManyDefaults. I'd expected it to be 5 (1 def, 1 explicitly specified operands, 2 default operands from m1Z, and another from Z) but it comes out as 4 (1 def, 3 uses). As a result, the code is mis-counting the number of MO's to expect and the number that are supplied and therefore adds the wrong number of default operands.
> > > 
> > > I've also noticed that this patch unconditionally adds the default operands this will lead to duplicated operands if a pattern supplies them. I'm currently working on a patch to fix both cases.
> > Hmm. I'm sure I encountered a case where an OperandWithOptionalOps shouldn't insert the DefaultOps because that's what caused me to write this code but I'm unable to find it now and my attempts at supplying the optional operands in a pattern have failed.
> > 
> > If you agree that OperandWithOptionalOps and friends are really operands that are never provided in a pattern then I think we should add them unconditionally as per this patch. We should also be able to remove this loop and check isSubClassOf("OperandWithDefaultOps") where we currently have DefaultOperands.count(I)
> On ARM you can look at ADD / SUB.
> I agree that DstI.Operands are a bit fishy, I'm not sure how they interact with ComplexPatterns. In any case, without a relevant test we should just go with the simplest code for now and do our best to detect funny situations. I'll have another stab at it without the loop. 
> In any case, without a relevant test we should just go with the simplest code for now and do our best to detect funny situations.

Yep, I agree.


https://reviews.llvm.org/D33031