[PATCH] D111506: [GlobalISel][Tablegen] Fix SameOperandMatcher's isIdentical check

Thu Oct 28 03:11:47 PDT 2021

kschwarz added inline comments.

================
Comment at: llvm/test/TableGen/GlobalISelEmitterMatchTableOptimizerSameOperand-invalid.td:16
+// CHECK-NEXT:        GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
+// CHECK-NEXT:        GIM_CheckIsSameOperand, /*MI*/0, /*OpIdx*/3, /*OtherMI*/2, /*OtherOpIdx*/2,
+// CHECK-NEXT:        GIM_Try, /*On fail goto*//*Label 2*/ 108, // Rule ID 1 //
----------------
qcolombet wrote:
> kschwarz wrote:
> > Even with the fix for the `isSameOperand` predicate, we can still have a situation where hoisting the check is not correct.
> > 
> > In this case, the optimizer finds that the operand checks are indeed identical, and hoists the check into a common block.
> > However, the check will unconditionally access operand index 2 of `/*OtherMI*/2`.
> > If the input GMIR does not have three operands in `/*OtherMI*/2`, this will crash.
> > 
> > The safest way would be to forbid hoisting `SameOperandMatcher`s altogether.
> > Anyone have a better idea?
> Just to make sure I understand correctly: what you're saying is this patch fixes one problem but there are others that need to be fixed as well, but this is orthogonal to this patch.
> 
> Is that right?
Exactly. This patch fixes the issue where two SameOperandMatchers are considered identical, although they are not.

The other issue that is not addressed yet is that the table optimizer will hoist identical checks, even though it might not always be safe to do so.
One such case is shown in this "invalid" test, where the `GIM_CheckIsSameOperand` at line 16 is hoisted and thus also executed on the path to the matcher at line 75 (Label 1), which can lead to a crash of the compiler.

I'll try to come up with a patch for that in a follow-up.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D111506/new/

https://reviews.llvm.org/D111506