This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] add folds for icmp (smin X, Y), X
ClosedPublic

Authored by spatel on Dec 7 2016, 11:30 AM.

Details

Summary

As Volkan noted in the commit thread for rL287585, min/max canonicalization exposes the fact that we're missing combines for min/max patterns. This patch won't solve the example that was attached to that thread, so something else still needs fixing.

The line between InstCombine and InstSimplify gets blurry here because sometimes the icmp instruction that we want to fold to already exists, but sometimes it's the swapped form of what we want.

Also note that there's yet another definition of min/max via the matcher patterns (I didn't find these until recently). I'm still hoping to canonicalize the non-standard patterns detected by matchSelectPattern(), so I went ahead and used m_SMin here even though I think that means those non-standard patterns will fall through for now.

Once this looks ok, I can follow up with the corresponding smax/umin/umax.

Diff Detail

Event Timeline

spatel updated this revision to Diff 80630.Dec 7 2016, 11:30 AM
spatel retitled this revision from to [InstCombine] add folds for icmp (smin X, Y), X.
spatel updated this object.
spatel added reviewers: efriedma, majnemer, jmolloy.
spatel added subscribers: volkan, llvm-commits.
spatel updated this revision to Diff 80649.Dec 7 2016, 1:24 PM

Patch updated:
It makes the patch smaller (static function, no need for a switch) to not consider the fold to true/false cases that are obviously handled by InstSimplify, so I removed that part.

majnemer added inline comments.Dec 7 2016, 1:32 PM
lib/Transforms/InstCombine/InstCombineCompares.cpp
3040–3041

Might be nice to have a m_c_SMin or something, would simplify this expression and the one bellow.

spatel updated this revision to Diff 80684.Dec 7 2016, 4:06 PM

Patch updated:
Add and use a commutable SMin matcher as suggested by David.

jmolloy accepted this revision.Dec 14 2016, 11:34 PM
jmolloy edited edge metadata.

LGTM!

This revision is now accepted and ready to land.Dec 14 2016, 11:34 PM
This revision was automatically updated to reflect the committed changes.