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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69217