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

[Diana Picus via Phabricator via llvm-commits]

rovka created this revision.
Herald added subscribers: igorb, kristof.beyls, rengolin, aemerson.

When looping through a destination pattern's operands to decide how many
default operands we need to introduce, we used to count the "expanded"
number of operands. So if one default operand would be rendered as 2
values, we'd count it as 2 operands, when in fact it needs to count as
only 1 operand regardless of how many values it expands to.

This turns out to be a problem only in some very specific cases, e.g.
when we have one operand with multiple default values followed by more
operands with default values (see the new test). In such a situation
we'd stop looping before looking at all the operands, and then error out
assuming that we don't have enough default operands to make up the
shortfall.

At the moment this only affects ARM.


https://reviews.llvm.org/D33031

Files:
  test/TableGen/GlobalISelEmitter.td
  utils/TableGen/GlobalISelEmitter.cpp


Index: utils/TableGen/GlobalISelEmitter.cpp
===================================================================
--- utils/TableGen/GlobalISelEmitter.cpp
+++ utils/TableGen/GlobalISelEmitter.cpp
@@ -1514,16 +1514,10 @@
   // enough operands to render the instruction.
   SmallSet<unsigned, 2> DefaultOperands;
   unsigned DstINumUses = DstI.Operands.size() - DstI.Operands.NumDefs;
-  unsigned NumDefaultOperands = 0;
-  for (unsigned I = 0; I < DstINumUses &&
-                       DstINumUses > Dst->getNumChildren() + NumDefaultOperands;
-       ++I) {
+  for (unsigned I = 0; I < DstINumUses; ++I) {
     const auto &DstIOperand = DstI.Operands[DstI.Operands.NumDefs + I];
-    if (DstIOperand.Rec->isSubClassOf("OperandWithDefaultOps")) {
+    if (DstIOperand.Rec->isSubClassOf("OperandWithDefaultOps"))
       DefaultOperands.insert(I);
-      NumDefaultOperands +=
-          DstIOperand.Rec->getValueAsDag("DefaultOps")->getNumArgs();
-    }
   }
   if (DstINumUses > Dst->getNumChildren() + DefaultOperands.size())
     return failedImport("Insufficient operands supplied and default ops "
Index: test/TableGen/GlobalISelEmitter.td
===================================================================
--- test/TableGen/GlobalISelEmitter.td
+++ test/TableGen/GlobalISelEmitter.td
@@ -387,6 +387,42 @@
 def XORlike : I<(outs GPR32:$dst), (ins m1Z:$src2, GPR32:$src1),
                 [(set GPR32:$dst, (xor GPR32:$src1, -4))]>;
 
+//===- Test a simple pattern with multiple operands with defaults. --------===//
+//
+
+// CHECK-LABEL: if ([&]() {
+// CHECK-NEXT:    MachineInstr &MI0 = I;
+// CHECK-NEXT:    if (MI0.getNumOperands() < 3)
+// CHECK-NEXT:      return false;
+// CHECK-NEXT:    if ((MI0.getOpcode() == TargetOpcode::G_XOR) &&
+// CHECK-NEXT:        ((/* dst */ (MRI.getType(MI0.getOperand(0).getReg()) == (LLT::scalar(32))) &&
+// CHECK-NEXT:         ((&RBI.getRegBankFromRegClass(MyTarget::GPR32RegClass) == RBI.getRegBank(MI0.getOperand(0).getReg(), MRI, TRI))))) &&
+// CHECK-NEXT:        ((/* src1 */ (MRI.getType(MI0.getOperand(1).getReg()) == (LLT::scalar(32))) &&
+// CHECK-NEXT:         ((&RBI.getRegBankFromRegClass(MyTarget::GPR32RegClass) == RBI.getRegBank(MI0.getOperand(1).getReg(), MRI, TRI))))) &&
+// CHECK-NEXT:        ((/* Operand 2 */ (MRI.getType(MI0.getOperand(2).getReg()) == (LLT::scalar(32))) &&
+// CHECK-NEXT:        (isOperandImmEqual(MI0.getOperand(2), -5, MRI))))) {
+// CHECK-NEXT:      // (xor:i32 GPR32:i32:$src1, -5:i32) => (XORManyDefaults:i32 GPR32:i32:$src1)
+// CHECK-NEXT:      MachineInstrBuilder MIB = BuildMI(*I.getParent(), I, I.getDebugLoc(), TII.get(MyTarget::XORManyDefaults));
+// CHECK-NEXT:      MIB.add(MI0.getOperand(0)/*dst*/);
+// CHECK-NEXT:      MIB.addImm(-1);
+// CHECK-NEXT:      MIB.addReg(MyTarget::R0);
+// CHECK-NEXT:      MIB.addReg(MyTarget::R0);
+// CHECK-NEXT:      MIB.add(MI0.getOperand(1)/*src1*/);
+// CHECK-NEXT:      for (const auto *FromMI : {&MI0, })
+// CHECK-NEXT:        for (const auto &MMO : FromMI->memoperands())
+// CHECK-NEXT:          MIB.addMemOperand(MMO);
+// CHECK-NEXT:      I.eraseFromParent();
+// CHECK-NEXT:      MachineInstr &NewI = *MIB;
+// CHECK-NEXT:      constrainSelectedInstRegOperands(NewI, TII, TRI, RBI);
+// CHECK-NEXT:      return true;
+// CHECK-NEXT:    }
+// CHECK-NEXT:    return false;
+// CHECK-NEXT:  }()) { return true; }
+
+// The -5 is just to distinguish it from the other cases.
+def XORManyDefaults : I<(outs GPR32:$dst), (ins m1Z:$src3, Z:$src2, GPR32:$src1),
+                        [(set GPR32:$dst, (xor GPR32:$src1, -5))]>;
+
 //===- Test a simple pattern with constant immediate operands. ------------===//
 //
 // This must precede the 3-register variants because constant immediates have


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D33031.98408.patch
Type: text/x-patch
Size: 3737 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170510/7ce6ab86/attachment.bin>