This is an archive of the discontinued LLVM Phabricator instance.

[IR][PatternMatch] introduce m_Unless() matcher
ClosedPublic

Authored by lebedev.ri on Jul 1 2019, 2:30 PM.

Details

Summary

I don't think it already exists? I don't see it at least.
It is important to have it because else we'll do some checks after match(),
and that may result in missed folds in commutative nodes.

Diff Detail

Repository
rL LLVM

Event Timeline

Do you have a specific usecase in mind?

Do you have a specific usecase in mind?

Yes, absolutely. I don't submit patches otherwise.

Do you have a specific usecase in mind?

Yes, absolutely. I don't submit patches otherwise.

Please can you update the dependency on this patch to point to the usecases?

This comment was removed by lebedev.ri.

Do you have a specific usecase in mind?

Yes, absolutely. I don't submit patches otherwise.

Please can you update the dependency on this patch to point to the usecases?

I didn't post that patch yet. It comes up when fixing PR42466
We have icmp eq/ne (and (shl X, Y), Z), 0, and if X is not a constant we want to turn it into icmp eq/ne (and (lshr Z, Y), X), 0.
But the problem is that if we do the obvious thing and directly patternmatch that, and *then* check that X is not a constant (and not fold if it is),
we then won't check whether Z is also a (shl Q, W) with Q not being a constant, and this we have missed commutative case..

Do you have a specific usecase in mind?

Yes, absolutely. I don't submit patches otherwise.

Please can you update the dependency on this patch to point to the usecases?

I didn't post that patch yet. It comes up when fixing PR42466
We have icmp eq/ne (and (shl X, Y), Z), 0, and if X is not a constant we want to turn it into icmp eq/ne (and (lshr Z, Y), X), 0.
But the problem is that if we do the obvious thing and directly patternmatch that, and *then* check that X is not a constant (and not fold if it is),
we then won't check whether Z is also a (shl Q, W) with Q not being a constant, and this we have missed commutative case..

OK I'm convinced, does anyone else have any comments?

spatel accepted this revision.Jul 8 2019, 9:56 AM

LGTM

unittests/IR/PatternMatch.cpp
89 ↗(On Diff #207410)

Better to call this value 'X' or some other non-overlapping name - easy to misread 'm_One' and 'One' below here.

This revision is now accepted and ready to land.Jul 8 2019, 9:56 AM

Thanks for the review!

lebedev.ri marked an inline comment as done.

Rebased, NFC.

This revision was automatically updated to reflect the committed changes.