[GlobalISel] Match table opt: fix a bug in matching num of operands

[rtereshin]

Summary

If there is a dag node with a variable number of operands that has at
least N operands (for some non-negative N), and multiple patterns with
that node with different number of operands, we would drop the number of
operands check in patterns with N operands, presumably because it's
guaranteed in such case that none of the per-operand checks will access
the operand list out-of-bounds.

Except semantically the check is about having exactly N operands, not at
least N operands, and a backend might rely on it to disambiguate
different patterns.

In this patch we change the condition on emitting the number of operands
check from "the instruction is not guaranteed to have at least as many
operands as are checked by the pattern being matched" to "the
instruction is not guaranteed to have a specific number of operands".

We're relying (still) on the rest of the CodeGenPatterns mechanics to
validate that the pattern itself doesn't try to access more operands
than there is in the instruction in cases when the instruction does have
fixed number of operands, and on the machine verifier to validate at
runtime that particular MIs like that satisfy the constraint as well.

[arsenm]

Testcase?

[rtereshin]

In D69653#1728175, @arsenm wrote:

Testcase?

Doesn't exist upstream. Match tables gain about one extra num operands' check (for G_INTRINSIC_W_SIDE_EFFECTS), but it doesn't affect the behavior as far as I can tell, as in those cases the number of operands semantically is in fact stable, just not evident from static properties of the generic opcode itself.

[qcolombet]

For the test case, couldn't you add a isVariadic instruction with different patterns in one of the TableGen test?

Otherwise LGTM.

llvm/utils/TableGen/GlobalISelEmitter.cpp

2276

Stupid question: what complains if an instruction does not have the isVariadic attribute, but we write patterns with different number of operands?
(Just wondering if we need an assert that !isVariadic => getNumOperands() == OpcMatcher.getNumOperands()).

[rtereshin]

llvm/utils/TableGen/GlobalISelEmitter.cpp

2276

Stupid question: what complains if an instruction does not have the isVariadic attribute, but we write patterns with different number of operands?

This does: https://github.com/llvm/llvm-project/blob/master/llvm/utils/TableGen/CodeGenDAGPatterns.cpp#L2414-L2420

(Just wondering if we need an assert that !isVariadic => getNumOperands() == OpcMatcher.getNumOperands()).

Considered the same, decided redundant.

[rtereshin]

Added a TableGen test as requested, though, I'm quite convinced it wasn't worth the time and will be difficult to maintain.

[qcolombet]

Thanks for the test case!

llvm/utils/TableGen/GlobalISelEmitter.cpp

2276

Sounds good.

llvm/test/TableGen/GlobalISelEmitterVariadic.td

+// RUN: llvm-tblgen -gen-global-isel -I %p/../../include -I %p/Common %s -o - | FileCheck %s
+
+include "llvm/Target/Target.td"
+include "GlobalISelEmitterCommon.td"
+
+def : GINodeEquiv<G_BUILD_VECTOR, build_vector>;
+
+def ONE : I<(outs GPR32:$dst), (ins GPR32:$src1), []>;
+def TWO : I<(outs GPR32:$dst), (ins GPR32:$src1, GPR32:$src2), []>;
+
+// G_BUILD_VECTOR is guaranteed to have at least one operand, therefore performing a
+// number of operands check is not needed to avoid per-operand checks accessing the
+// MI's operand list out of bounds in this particular pattern. However, we still
+// must perform the check, as a G_BUILD_VECTOR instance with two source operands
+// will pass all checks done by this pattern otherwise, which will lead to a
+// mis-match if this pattern tried first (and it will if it has higher complexity).
+def : Pat<(build_vector GPR32:$src1),
+          (ONE GPR32:$src1)> {
+  let AddedComplexity = 1000;
+}
+
+def : Pat<(build_vector GPR32:$src1, GPR32:$src2),
+          (TWO GPR32:$src1, GPR32:$src2)>;
+
+// CHECK:       GIM_Try, /*On fail goto*//*Label 0*/ [[NEXT_OPCODE_LABEL:[0-9]+]],
+// CHECK-NEXT:    GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_BUILD_VECTOR,
+// CHECK-NEXT:    GIM_Try, /*On fail goto*//*Label 1*/ [[NEXT_NUM_OPERANDS_LABEL_1:[0-9]+]], // Rule ID 0 //
+// CHECK-NEXT:      GIM_CheckNumOperands, /*MI*/0, /*Expected*/2,
+// CHECK-NEXT:      GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
+// CHECK-NEXT:      GIM_CheckType, /*MI*/0, /*Op*/1, /*Type*/GILLT_s32,
+// CHECK-NEXT:      GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK-NEXT:      GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK-NEXT:      // (build_vector:{ *:[i32] } GPR32:{ *:[i32] }:$src1)  =>  (ONE:{ *:[i32] } GPR32:{ *:[i32] }:$src1)
+// CHECK-NEXT:      GIR_MutateOpcode, /*InsnID*/0, /*RecycleInsnID*/0, /*Opcode*/MyTarget::ONE,
+// CHECK-NEXT:      GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT:      // GIR_Coverage, 0,
+// CHECK-NEXT:      GIR_Done,
+// CHECK-NEXT:    // Label 1: @[[NEXT_NUM_OPERANDS_LABEL_1]]
+// CHECK-NEXT:    GIM_Try, /*On fail goto*//*Label 2*/ [[NEXT_NUM_OPERANDS_LABEL_2:[0-9]+]], // Rule ID 1 //
+// CHECK-NEXT:      GIM_CheckNumOperands, /*MI*/0, /*Expected*/3,
+// CHECK-NEXT:      GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
+// CHECK-NEXT:      GIM_CheckType, /*MI*/0, /*Op*/1, /*Type*/GILLT_s32,
+// CHECK-NEXT:      GIM_CheckType, /*MI*/0, /*Op*/2, /*Type*/GILLT_s32,
+// CHECK-NEXT:      GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK-NEXT:      GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK-NEXT:      GIM_CheckRegBankForClass, /*MI*/0, /*Op*/2, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK-NEXT:      // (build_vector:{ *:[i32] } GPR32:{ *:[i32] }:$src1, GPR32:{ *:[i32] }:$src2)  =>  (TWO:{ *:[i32] } GPR32:{ *:[i32] }:$src1, GPR32:{ *:[i32] }:$src2)
+// CHECK-NEXT:      GIR_MutateOpcode, /*InsnID*/0, /*RecycleInsnID*/0, /*Opcode*/MyTarget::TWO,
+// CHECK-NEXT:      GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT:      // GIR_Coverage, 1,
+// CHECK-NEXT:      GIR_Done,
+// CHECK-NEXT:    // Label 2: @[[NEXT_NUM_OPERANDS_LABEL_2]]
+// CHECK-NEXT:    GIM_Reject,
+// CHECK-NEXT:  // Label 0: @[[NEXT_OPCODE_LABEL]]
+// CHECK-NEXT:  GIM_Reject,

llvm/utils/TableGen/GlobalISelEmitter.cpp

isHigherPriorityThan(const InstructionPredicateMatcher &B) const override {
     return false;
   };
 
   bool isConstantInstruction() const {
     return I->TheDef->getName() == "G_CONSTANT";
   }
 
   StringRef getOpcode() const { return I->TheDef->getName(); }
-  unsigned getNumOperands() const { return I->Operands.size(); }
+  bool isVariadicNumOperands() const { return I->Operands.isVariadic; }
 
   StringRef getOperandType(unsigned OpIdx) const {
     return I->Operands[OpIdx].OperandType;
   }
 };
 
 DenseMap<const CodeGenInstruction *, unsigned>
     InstructionOpcodeMatcher::OpcodeValues;

 };
 
 void InstructionMatcher::optimize() {
   SmallVector<std::unique_ptr<PredicateMatcher>, 8> Stash;
   const auto &OpcMatcher = getOpcodeMatcher();
 
   Stash.push_back(predicates_pop_front());
   if (Stash.back().get() == &OpcMatcher) {
-    if (NumOperandsCheck && OpcMatcher.getNumOperands() < getNumOperands())
+    if (NumOperandsCheck && OpcMatcher.isVariadicNumOperands())
       Stash.emplace_back(
           new InstructionNumOperandsMatcher(InsnVarID, getNumOperands()));
     NumOperandsCheck = false;
 
     for (auto &OM : Operands)
       for (auto &OP : OM->predicates())
         if (isa<IntrinsicIDOperandMatcher>(OP)) {
           Stash.push_back(std::move(OP));