This is an archive of the discontinued LLVM Phabricator instance.

Refactor implied condition logic from ValueTracking directly into CmpInst. NFC.
ClosedPublic

Authored by mcrosier on Apr 20 2016, 9:03 AM.

Details

Summary

This logic allows a compare instruction to infer the condition of another compare with matching operands without having to use ValueTracking directly.

Chad

Diff Detail

Event Timeline

mcrosier updated this revision to Diff 54380.Apr 20 2016, 9:03 AM
mcrosier retitled this revision from to Refactor implied condition logic from ValueTracking directly into CmpInst. NFC..
mcrosier updated this object.
mcrosier added reviewers: sanjoy, gberry, reames.
mcrosier added a subscriber: llvm-commits.
sanjoy requested changes to this revision.Apr 20 2016, 10:56 AM
sanjoy edited edge metadata.
sanjoy added inline comments.
lib/Analysis/ValueTracking.cpp
3845

Can't the logic in this switch also be moved to CmpInst::isTrueWhenOperandsMatch and CmpInst::isFaseWhenOperandsMatch ?

This revision now requires changes to proceed.Apr 20 2016, 10:56 AM
mcrosier updated this revision to Diff 54405.Apr 20 2016, 12:52 PM
mcrosier edited edge metadata.

Move the remaining bit of logic into CmpInst, per Sanjoy's suggestion.

sanjoy accepted this revision.Apr 20 2016, 10:54 PM
sanjoy edited edge metadata.

Given that this passes all the checked in tests then I'm okay signing off on this for now (with a FIXME stating what remains to be done); but depending on how much time you're willing to commit here, I think we can make this code more comprehensive. I've added a comment inline about some of the missing cases, but there is a useful litmus test here: we should be able to implement isFalseWhenOperandsMatch(X, Y) exactly as isTrueWhenOperandsMatch(X, getInversePredicate(Y)). Any situation where that doesn't work is a missing case.

lib/IR/Instructions.cpp
3609

UGT also implies UGE etc.

EQ will imply all UGE, SLE etc. variants.

This revision is now accepted and ready to land.Apr 20 2016, 10:54 PM
This revision was automatically updated to reflect the committed changes.
mcrosier added a comment.EditedApr 21 2016, 7:14 AM

Given that this passes all the checked in tests then I'm okay signing off on this for now (with a FIXME stating what remains to be done); but depending on how much time you're willing to commit here, I think we can make this code more comprehensive. I've added a comment inline about some of the missing cases, but there is a useful litmus test here: we should be able to implement isFalseWhenOperandsMatch(X, Y) exactly as isTrueWhenOperandsMatch(X, getInversePredicate(Y)). Any situation where that doesn't work is a missing case.

Yes, the code can be more comprehensive. I'll post a patch shortly as I'd like for this work to be complete. Thanks, Sanjoy.