[PATCH] D69217: [ConstantRange] makeGuaranteedNoWrapRegion(): `shl` support

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/IR/ConstantRange.cpp:276
+    if (ShAmtUMax.uge(BitWidth))
+      return getEmpty(BitWidth); // Shift by bitwidth or more always overflows.
+    if (Unsigned)
----------------
nikic wrote:
> lebedev.ri wrote:
> > lebedev.ri wrote:
> > > nikic wrote:
> > > > While an empty set here is conservatively correct, I believe that the more precise return value would be the range containing only zero `[0, 1)`. The shift amount is < the bitwidth by assumption (otherwise the result is poison), and `[0, 1)` is the result for the worst-case assumption of shift amount = bitwidth - 1.
> > > > 
> > > > Even more precise would be to intersect the incoming range with `[0, BitWidth)`, which may differ non-trivially from this approach for wrapped ranges. E.g. if you have a signed range `[INT_MIN, 5)`, the UMax will be `UINT_MAX`, but the intersection `[INT_MIN, 5) /\ [0, 32)` is `[0, 5)` and the UMax will be `4`, resulting in a much larger guaranteed nowrap region.
> > > > The shift amount is < the bitwidth by assumption (otherwise the result is poison)
> > > 
> > > Ah, good point.
> > > 
> > > But by that logic, then for signed the range should be `[-1, 1)`, correct?
> > > `0 << 7 == 0` - doesn't overflow in both signed and unsigned domains,
> > > while `-1 << 7 == -128` does not overflow in signed domain - still the same sign.
> > Actually no, wait, if we are assuming that we will at worst get bitwidth-1 shift amount,
> > then we should produce `[0, 2)` for unsigned or `[-1, 1)` for signed.
> > Or i'm missing the point.
> I guess it should be `[0, 2)` unsigned and `[-1, 1)` signed then, as `1 << 7` does not overflow unsigned either.
> 
> Rather than special-casing this, we could just take `ShAmtUMax = Other.getUnsignedMax().umin(BitWidth-1)` and reuse the general code.
> 
> Or do
> 
> ```
> ConstantRange ShAmt = Other.intersect(ConstantRange(
>     APInt(BitWidth, 0), APInt(BitWidth, BitWidth)));`
> if (ShAmt.isEmptySet())
>   return getEmpty(); // or getFull()?
> APInt ShAmtUMax = ShAmt.getUnsignedMax();
> ```
> 
> for the fully precise variant.
> I guess it should be [0, 2) unsigned and [-1, 1) signed then, as 1 << 7 does not overflow unsigned either.
> Rather than special-casing this, we could just take ShAmtUMax = Other.getUnsignedMax().umin(BitWidth-1) and reuse the general code.

Indeed, which is why i was having trouble grasping why it would be `[0, 1)` for shamt=bitwidth all of a sudden. 

Didn't think about intersection yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69217