This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] (icmp sgt smin(PosA, B) 0) -> (icmp sgt B 0)
ClosedPublic

Authored by reames on Mar 3 2016, 5:45 PM.

Details

Summary

When checking whether an smin is positive, we can move the comparison to one of the inputs if the other is known positive. If the known positive one is the min, then the other can't be negative. If the other is the min, then we compute the min.

Diff Detail

Event Timeline

reames updated this revision to Diff 49789.Mar 3 2016, 5:45 PM
reames retitled this revision from to [InstCombine] (icmp sgt smin(PosA, B) 0) -> (icmp sgt B 0).
reames updated this object.
reames added reviewers: sanjoy, majnemer.
reames added a subscriber: llvm-commits.
sanjoy requested changes to this revision.Mar 3 2016, 5:58 PM
sanjoy edited edge metadata.
sanjoy added inline comments.
lib/Transforms/InstCombine/InstCombineCompares.cpp
3180

Why not just use an if here?

3183

I think this needs to be isKnownPositive, since (sgt (smin 0 1) 0) == false but (sgt 1 0) == true.

This revision now requires changes to proceed.Mar 3 2016, 5:58 PM
reames added inline comments.Mar 3 2016, 6:17 PM
lib/Transforms/InstCombine/InstCombineCompares.cpp
3183

You are correct.

reames updated this revision to Diff 49846.Mar 4 2016, 1:19 PM
reames edited edge metadata.

Updated patch which fixed bug Sanjoy pointed out

sanjoy requested changes to this revision.Mar 4 2016, 1:33 PM
sanjoy edited edge metadata.
sanjoy added inline comments.
lib/Transforms/InstCombine/InstCombineCompares.cpp
3181

This is more work for you, but I think the right thing to do here is introduce an isKnownPositive helper in ValueTracking.h -- doing two recursive walks on the same expression seems wasteful.

I'd say lets start with a simple isKnownPositive implementation (maybe one that just looks at !range metadata and computeKnownBits), which we can evolve in the future.

This revision now requires changes to proceed.Mar 4 2016, 1:33 PM
reames added inline comments.Mar 4 2016, 1:42 PM
lib/Transforms/InstCombine/InstCombineCompares.cpp
3181

I disagree that introducing yet another independent implementation is a good idea.

To be blunt, the extra work to make the approach you're requesting work isn't worth it for this patch. I'll just abandon the patch instead because it turns out not to address the case I actually care about.

sanjoy added inline comments.Mar 4 2016, 2:42 PM
lib/Transforms/InstCombine/InstCombineCompares.cpp
3181

I disagree that introducing yet another independent implementation is a good idea.

I grepped around in the codebase, and it looks like SimplifyICmpInst already has this idiom (i.e. isKnownNonNegative(X) followed by isKnownNonZero(X)), so I suppose one more instance of it can't hurt. The avoidable 6-deep traversal still irks me though -- maybe we can leave a FIXME here?

sanjoy accepted this revision.Mar 4 2016, 2:42 PM
sanjoy edited edge metadata.
This revision is now accepted and ready to land.Mar 4 2016, 2:42 PM
This revision was automatically updated to reflect the committed changes.