Page MenuHomePhabricator

[ConstantRange] Add makeExactNoWrapRegion()

Authored by nikic on Apr 22 2019, 2:45 AM.



I got confused on the terminology, and the change in D60598 was not correct. I was thinking of "exact" in terms of the result being non-approximate. However, the relevant distinction here is whether the result is

  • Largest range such that: Forall Y in Other: Forall X in Result: X BinOp Y does not wrap. (makeGuaranteedNoWrapRegion)
  • Smallest range such that: Forall Y in Other: Forall X not in Result: X BinOp Y wraps. (A hypothetical makeAllowedNoWrapRegion)
  • Both. (makeExactNoWrapRegion)

I'm adding a separate makeExactNoWrapRegion method accepting a single APInt (same as makeExactICmpRegion) and using it in the places where the guarantee is relevant.

Sorry for the confusion...

Diff Detail


Event Timeline

nikic created this revision.Apr 22 2019, 2:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 22 2019, 2:45 AM

Does this needs tests?

nikic updated this revision to Diff 196047.Apr 22 2019, 3:25 AM

Add (indirect) test. In the makeGuaranteedNoWrapRegion() tests check that the result for single-element ranges is indeed exact.

Hmm, it's concerning that i missed the difference in the original change.
Could someone else please also take a look here?

sanjoy accepted this revision.Apr 27 2019, 11:53 AM

I got confused on the terminology

Any suggestions on how we can improve the terminology to be less confusing?

127 ↗(On Diff #196047)

Does this always return the *largest* such range? (Just double-checking that this language is intentional.)

147 ↗(On Diff #196047)

Up to you, but there is a standard way of describing this: "Returns the range such that X is in returned range if and only if X binop Y does not overflow."

This revision is now accepted and ready to land.Apr 27 2019, 11:53 AM
This revision was automatically updated to reflect the committed changes.
nikic marked 3 inline comments as done.
nikic added inline comments.Apr 28 2019, 8:40 AM
127 ↗(On Diff #196047)

Yep, this is indeed the largest range. We have some tests to check that.