This is an archive of the discontinued LLVM Phabricator instance.

[LVI] Make LVI smarter about comparisons with non-constants
ClosedPublic

Authored by apilipenko on Aug 5 2016, 7:16 AM.

Details

Summary

Make LVI smarter about comparisons with a non-constant. For example, a s< b constraints a to be in [INT_MIN, INT_MAX) range. This is a part of https://llvm.org/bugs/show_bug.cgi?id=28620 fix.

Note that this patch is based on D23200 and few minor refactorings in getValueFromICmpCondition (extraction of RHS, LHS, Predicate locals, renaming NegOffset to Offset) which were not included to simplify the review.

Diff Detail

Event Timeline

apilipenko updated this revision to Diff 66944.Aug 5 2016, 7:16 AM
apilipenko retitled this revision from to [LVI] Make LVI smarter about comparisons with non-constants.
apilipenko updated this object.
apilipenko added reviewers: reames, sanjoy.
apilipenko added a subscriber: llvm-commits.
sanjoy requested changes to this revision.Aug 5 2016, 12:46 PM
sanjoy edited edge metadata.
sanjoy added inline comments.
lib/Analysis/LazyValueInfo.cpp
1205

Do you mean icmp ult Val, ...?

1220–1221

Please make this change to how we implement !isTrueDest separately. IMO it _should_ be NFC, but is tricky enough that it should not go in with other changes.

[edit: I understand that *with your change* this isn't NFC, and actually needed for correctness -- so I think this should go in separately as NFC before this change since given the state of LVI today it is NFC.]

1229

Can you please explicitly pass in /*isFullSet=*/true to the constructor? The fact that RHSRange is the full set is important here.

1232–1233

Minor: a ternary operator for Pred may be more readable here.

This revision now requires changes to proceed.Aug 5 2016, 12:46 PM
apilipenko updated this revision to Diff 67168.Aug 8 2016, 7:48 AM
apilipenko edited edge metadata.

Address review comments, false dest handling was landed separately as NFC.

apilipenko marked 2 inline comments as done.Aug 8 2016, 7:50 AM
apilipenko added inline comments.
lib/Analysis/LazyValueInfo.cpp
1205

No, this code can handle any predicate.

The predicate limitation for icmp ult (add Val, Offset), ... conditions is somewhat artificial it comes from the existing code. As a separate change I'm going to support arbitrary predicates for conditions in this form as well.

lgtm!

lib/Analysis/LazyValueInfo.cpp
1205

In that case, I think

icmp <pred> Val, ...

will be clearer.

sanjoy accepted this revision.Aug 8 2016, 2:35 PM
sanjoy edited edge metadata.

lgtm again (forgot to set 'Accept Revision' the last time).

This revision is now accepted and ready to land.Aug 8 2016, 2:35 PM
This revision was automatically updated to reflect the committed changes.
apilipenko marked an inline comment as done.