Page MenuHomePhabricator

[GlobalISel][Tablegen] Fix SameOperandMatcher's isIdentical check

Authored by kschwarz on Oct 10 2021, 2:31 AM.



During rule optimization, identical SameOperandMatchers are hoisted into a common group,
however previously only one operand index was considered.
Commutable patterns can introduce SameOperandMatcher checks where the second index is commuted,
resulting in a different check that cannot be hoisted.

Diff Detail

Event Timeline

kschwarz created this revision.Oct 10 2021, 2:31 AM
kschwarz requested review of this revision.Oct 10 2021, 2:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2021, 2:31 AM
Herald added a subscriber: wdng. · View Herald Transcript
kschwarz updated this revision to Diff 381549.Oct 22 2021, 7:48 AM

Rebase & ping

kschwarz added inline comments.Oct 22 2021, 7:55 AM

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 SameOperandMatchers altogether.
Anyone have a better idea?

qcolombet accepted this revision.Oct 27 2021, 10:17 AM
qcolombet added inline comments.

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?

This revision is now accepted and ready to land.Oct 27 2021, 10:17 AM
kschwarz added inline comments.Oct 28 2021, 3:11 AM

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.

This revision was landed with ongoing or failed builds.Oct 28 2021, 4:37 AM
This revision was automatically updated to reflect the committed changes.