Page MenuHomePhabricator

Handle undef vectors consistently in pattern matching
ClosedPublic

Authored by nikic on Nov 16 2018, 7:04 AM.

Details

Summary

This patch fixes the issue noticed in D54532. The problem is that cst_pred_ty-based matchers like m_Zero() currently do not match scalar undefs (as expected), but *do* match vector undefs. This may lead to optimization inconsistencies in rare cases.

There is only one existing test for which output changes, reverting the change from D53205. The reason here is that vector fsub undef, %x is no longer matched as an m_FNeg(). While I think that the new output is technically worse than the previous one, it is consistent with scalar, and I don't think it's really important either way (generally that undef should have been folded away prior to reassociation.)

I've also added another test case for this issue based on InstructionSimplify. It took some effort to find that one, as in most cases undef folds are either checked first -- and in the cases where they aren't it usually happens to not make a difference in the end. This is the only case I was able to come up with. Prior to this patch the test case simplified to undef in the scalar case, but zeroinitializer in the vector case.

Diff Detail

Event Timeline

nikic created this revision.Nov 16 2018, 7:04 AM
lebedev.ri added inline comments.
include/llvm/IR/PatternMatch.h
230

So now instead of checking that any (zero or more) non-undef elements match,
we check that all (one or more) non-undef elements match?

In other words, if we had a vector of undefs, we used to accept it, and will no longer?
I think this needs unit tests. ({int, fp} x {scalar, vector})

test/Transforms/InstSimplify/shr-scalar-vector-consistency.ll
6

Please precommit the test and rebase.

nikic updated this revision to Diff 174380.Nov 16 2018, 8:36 AM
nikic edited the summary of this revision. (Show Details)

Add unit tests, rebase over committed baseline instsimplify test.

nikic added inline comments.Nov 16 2018, 9:57 AM
include/llvm/IR/PatternMatch.h
230

Yes, your understanding is exactly right. Previously all cst_pred_ty/cstfp_pred_ty matchers returned true for a vector of undefs, now they will return false.

I've now added some unit tests for behavior of m_Undef and m_Zero/m_AnyZeroFP applied to scalars and vectors of undef, zero and mixed zero/undef.

spatel accepted this revision.Nov 18 2018, 6:39 AM

LGTM - have you requested commit privileges? You should be able to get that based on approved patches here + D54531 + D54532.

This revision is now accepted and ready to land.Nov 18 2018, 6:39 AM
This revision was automatically updated to reflect the committed changes.