Page MenuHomePhabricator

[ConstantRange] Add makeExactNoWrapRegion()
ClosedPublic

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

Details

Summary

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

Repository
rL LLVM

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?

llvm/include/llvm/IR/ConstantRange.h
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
llvm/include/llvm/IR/ConstantRange.h
127 ↗(On Diff #196047)

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