Page MenuHomePhabricator

[IR] Add a `makeNoWrapRegion` method to `ConstantRange`
ClosedPublic

Authored by sanjoy on Oct 9 2015, 5:30 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 37008.Oct 9 2015, 5:30 PM
sanjoy retitled this revision from to [IR] Add a `makeNoWrapRegion` method to `ConstantRange`.
sanjoy updated this object.
sanjoy added reviewers: hfinkel, reames, nlewycky.
sanjoy added a subscriber: llvm-commits.
nlewycky edited edge metadata.Oct 9 2015, 6:09 PM

The logic looks correct, and I really like the test!

include/llvm/IR/ConstantRange.h
89–93 ↗(On Diff #37008)

Example in comments don't match function declaration? Did you want to make it an 'int WrappingFlags'? (I see that you actually use it as a bool, but I really like the named flags API shown in the comment much more.)

lib/IR/ConstantRange.cpp
138 ↗(On Diff #37008)

use BitWidth here

140 ↗(On Diff #37008)

C.isMinValue() is fully defined in the header.

151 ↗(On Diff #37008)

I think that's locally obvious enough that we don't need an assert. :)

sanjoy added inline comments.Oct 9 2015, 7:15 PM
include/llvm/IR/ConstantRange.h
89–93 ↗(On Diff #37008)

I initially did have it as a int WrappingFlags (and I forgot to update the comments when I changed this to use a bool), but using AddOperator::... here seemed arbitrary (for instance, is it okay to have BinOp be Instruction::Mul and pass in AddOperator::NoSignedWrap?

However, I share your preference for named parameters :), so I'll happily change this back to use AddOperator::NoUnsignedWrap if you're okay with it. I can also introduce an enum inside ConstantRange if that sounds better.

sanjoy updated this revision to Diff 37147.Oct 12 2015, 12:47 PM
sanjoy marked 4 inline comments as done.
sanjoy edited edge metadata.
  • address Nick's review
sanjoy added inline comments.Oct 12 2015, 12:48 PM
include/llvm/IR/ConstantRange.h
89–93 ↗(On Diff #37008)

Changed to use OverflowingBinaryOperator::NoXXXWrap.

nlewycky added inline comments.Oct 12 2015, 12:51 PM
lib/IR/ConstantRange.cpp
138–139 ↗(On Diff #37147)

Please sink this below the test for BinOp being Add. It's not a misuse of the API to call it with OBO::Exact or some other flag if the BinOp is not an add.

150 ↗(On Diff #37147)

Please handle the case where NoWrapKind = NoUnsignedWrap | NoSignedWrap.

sanjoy added inline comments.Oct 12 2015, 3:38 PM
lib/IR/ConstantRange.cpp
138–139 ↗(On Diff #37147)

But the API is called makeNoWrapRegion, so I'd say calling it with OBO::Exact or other flags unrelated to wrapping is a misuse.

(I don't have a very strong opinion on this, and will change it if you insist.

nlewycky added inline comments.Oct 12 2015, 5:00 PM
lib/IR/ConstantRange.cpp
138–139 ↗(On Diff #37147)

Oh, good point. I was imagining generalizing this function for all the optimization flags, but right now it's clearly labelled for wrapping. We can change that later when it's required.

sanjoy updated this revision to Diff 37207.Oct 12 2015, 6:18 PM
  • address review, and bugfix

I was using intersectWith incorrect. I've fixed the problem now (see PreciseIntersect).

nlewycky added inline comments.Oct 16 2015, 1:54 PM
include/llvm/IR/ConstantRange.h
91 ↗(On Diff #37207)

There is no OBO::UnsignedWrap??

lib/IR/ConstantRange.cpp
139 ↗(On Diff #37207)

ExclusiveIntersect? MinimalIntersect? SubsetIntersect? I don't feel like precise conveys meaning here (what's not precise about intersectWith? It's not about precision, it's about whether we choose to include all points in the intersection or choose to include only pointer in the intersection).

sanjoy updated this revision to Diff 37642.Oct 16 2015, 2:21 PM
sanjoy marked 2 inline comments as done.
  • address review

Addressed reviews

nlewycky accepted this revision.Oct 16 2015, 2:23 PM
nlewycky edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 16 2015, 2:23 PM
This revision was automatically updated to reflect the committed changes.