This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Use computeConstantRange() for signed add overflow determination
ClosedPublic

Authored by nikic on Apr 8 2019, 1:09 PM.

Details

Summary

This is D59386 for the signed add case. The computeConstantRange() result is now intersected into the existing known bits information, allowing to detect additional no-overflow/always-overflow conditions (though the latter isn't used yet).

This (finally...) covers the motivating case from D59071.

Diff Detail

Repository
rL LLVM

Event Timeline

nikic created this revision.Apr 8 2019, 1:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2019, 1:09 PM

Please can you split up your changes?
https://llvm.org/docs/DeveloperPolicy.html#code-reviews

Split your patch into multiple smaller patches that build on each other. The smaller your patch, the higher the probability that somebody will take a quick look at it.

I would very much prefer to quickly look through 3 small differentials here rather than one medium..

llvm/lib/Analysis/ValueTracking.cpp
4087–4091 ↗(On Diff #194198)

So the only thing happening here is that RangeType is now being passed, right?
Please, precommit the NFC part of the refactoring, and leave only

+ ConstantRange::PreferredRangeType RangeType =
+     ForSigned ? ConstantRange::Signed : ConstantRange::Unsigned;
- return CR1.intersectWith(CR2);
+ return CR1.intersectWith(CR2, RangeType);
4136–4159 ↗(On Diff #194198)

This looks like it should be a new differential, after the this one.

4195–4198 ↗(On Diff #194198)

And one more new differential

nikic updated this revision to Diff 194254.Apr 9 2019, 12:23 AM
nikic retitled this revision from [ValueTracking] Use computeConstantRange() for signed add/sub overflow determination to [ValueTracking] Use computeConstantRange() for signed add overflow determination.
nikic edited the summary of this revision. (Show Details)

Rebase over committed changes, reduce diff to only handling signed add.

nikic marked 6 inline comments as done.Apr 9 2019, 12:26 AM

There are a few additional test changes due to the min/max handling in computeConstantRange() that has landed in the meantime.

llvm/lib/Analysis/ValueTracking.cpp
4087–4091 ↗(On Diff #194198)

Done in rL357968.

4136–4159 ↗(On Diff #194198)

This change is needed because the LHSKnown variable no longer exists, so it can't come after. There's no reason it can't come before though, so done in rL357969.

4195–4198 ↗(On Diff #194198)

Done, I've reduced this to just the signed add case and will submit a followup for signed sub.

lebedev.ri added inline comments.Apr 9 2019, 12:34 AM
llvm/lib/Analysis/ValueTracking.cpp
4136–4159 ↗(On Diff #194198)

Sure. But i was talking about the entire block of code i highlighted with a comment.
I.e. i think the rest of the computeOverflowForSignedAdd() change should be in a new diff too.
Or if you leave just the computeConstantRangeIncludingKnownBits() part, there are no test changes?

nikic marked 4 inline comments as done.Apr 9 2019, 12:42 AM
nikic added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4136–4159 ↗(On Diff #194198)

Ooooh, I didn't realize that you can highlight a range of code and assumed your comment is about the change directly above.

But yes, just the computeConstantRangeIncludingKnownBits() change doesn't result in test changes (it it not strictly speaking NFC, but it's likely not possible to actually trigger a difference without using the function in a signed context).

lebedev.ri accepted this revision.Apr 9 2019, 4:43 AM
lebedev.ri marked an inline comment as done.

LG

llvm/lib/Analysis/ValueTracking.cpp
4136–4159 ↗(On Diff #194198)

Okay, thank you.

This revision is now accepted and ready to land.Apr 9 2019, 4:43 AM
This revision was automatically updated to reflect the committed changes.