[PATCH] D61084: [ConstantRange] Add abs() support

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

In D61084#1477747 <https://reviews.llvm.org/D61084#1477747>, @nikic wrote:

> In D61084#1477695 <https://reviews.llvm.org/D61084#1477695>, @lebedev.ri wrote:
>
> > The test certainly works, because we are only checking 4-bit values, while `APInt` is 64-bit or more.
> >  What happens for `ConstantRange`'s of larger width?
>
>
> The bit width doesn't make a difference here: If the signed min //for the particular bitwidth// is in the original range, it will also be in the result range.
>
> > Maybe you should return `ConstantRange` with bitwidth larger by one if `INT_MIN` is included?
>
> I don't think this would be particularly useful, at least in the context where this is intended to be used: The abs() implementation here models the semantics of SPF_ABS, which maps signed min onto signed min, and it's important to preserve that edge-case behavior.




> We have similar code for handling abs of signed min in ValueTracking: https://github.com/llvm-mirror/llvm/blob/4d4155f08ea87c6909ddd6db4610c3d6c8034983/lib/Analysis/ValueTracking.cpp#L5669-L5679

Okay, that clarifies things, thank you for pointing that out.
I believe ubsan will not complain here about negations, since `APInt::negate()` is always used.
LG.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61084/new/

https://reviews.llvm.org/D61084