Page MenuHomePhabricator

[InstCombine] Simplify icmp ult/uge (shl %x, C2), C1 iff C1 is power of two -> icmp eq/ne (and %x, (lshr -C1, C2)), 0.
ClosedPublic

Authored by huihuiz on Fri, Jun 21, 2:56 PM.

Diff Detail

Event Timeline

huihuiz created this revision.Fri, Jun 21, 2:56 PM

This will fix the missing fold found in D63505

Adding original test in differential D63670

lebedev.ri edited the summary of this revision. (Show Details)Sat, Jun 22, 1:45 AM

Nice, some comments.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
2032

This comment is for the second fold, for the first fold the comment is

// (X l<< C2) u<=/u> C1 iff C1+1 is power of two  ->  X & (~C1 l>> C2) ==/!= 0.
2033
if (Cmp.isUnsigned() && Shl.hasOneUse()) {
llvm/test/Transforms/InstCombine/shl-unsigned-cmp-const.ll
204–205

In this case, these folds should be limited to single-use shl, because we produce 2 instructions,
and only ensure that we replace a single instruction, thus we might increase instruction count.

lebedev.ri added inline comments.Sat, Jun 22, 3:48 AM
llvm/test/Transforms/InstCombine/shl-and-unsigned-cmp-const.ll
15–25 ↗(On Diff #206087)

Why is this test so complicated?

%shl = shl i8 %x, 5
%cmp = icmp ult i8 %shl, 64

seems sufficient?
https://godbolt.org/z/GcVzPz

lebedev.ri requested changes to this revision.Sun, Jun 23, 2:59 PM

(just marking as reviewed)

This revision now requires changes to proceed.Sun, Jun 23, 2:59 PM
huihuiz updated this revision to Diff 206309.Mon, Jun 24, 3:02 PM
huihuiz marked 4 inline comments as done.

addressed review comments

lebedev.ri accepted this revision.Mon, Jun 24, 3:13 PM
lebedev.ri marked an inline comment as done.
lebedev.ri added a subscriber: spatel.

Looks good, thank you.

llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
2041

Like i commented in tests, it is not really possible to guarantely test those predicates.
It seems like the predicates aren't being canonnicalized for vector compares, but that is likely a bug (CC @spatel)
So it would be great if you could add vector test coverage for these, for now at least.

This revision is now accepted and ready to land.Mon, Jun 24, 3:13 PM
spatel added inline comments.Mon, Jun 24, 3:31 PM
llvm/lib/Transforms/InstCombine/InstCombineCompares.cpp
2041

I haven't been following this set of patches closely, but lack of vector folds is still a fairly common problem. You can usually spot those limitations by searching for m_ConstantInt or dyn_cast to 'ConstantInt' rather than 'm_APInt'.

huihuiz updated this revision to Diff 206320.Mon, Jun 24, 4:12 PM

update for added vector tests

This revision was automatically updated to reflect the committed changes.