[PATCH] D71690: [SCEV] get a more accurate range for AddRecExpr with nuw flag

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5661
+        MinValue = getUnsignedRangeMin(AddRec->getStart());
+      else {
+        ConstantRange StartRange =
----------------
sanjoy.google wrote:
> Can you comment on why you need the else block?
If we use `getUnsignedRangeMin(AddRec->getStart())` as minimum value for SignedRange of the start of AddRec, we may get wrong result. For example, assuming we get range `[137, 227)` as result of `getUnsignedRangeMin(AddRec->getStart());` for type `i8`,  if we want to return it as SignedRange, it will be `[-119, -29)`, this is invalid range for AddRec with `nuw` even for SignedRange?


================
Comment at: llvm/test/Transforms/IndVarSimplify/lftr-pr31181.ll:332
 ; CHECK:       always_taken:
-; CHECK-NEXT:    [[IV2_INC]] = add nsw i32 [[IV2]], 1
+; CHECK-NEXT:    [[IV2_INC]] = add i32 [[IV2]], 1
 ; CHECK-NEXT:    [[IV_INC]] = add nsw i32 [[IV]], 1
----------------
nikic wrote:
> sanjoy.google wrote:
> > shchenz wrote:
> > > nikic wrote:
> > > > Aren't these two test changes regressions?
> > > Hi @nikic , thanks for your quick comment. For my understanding, seems the changes are improvement instead of regression. 
> > > Before LFTR, we use `%iv` as loop iteration variable, the loop count `loop_count` is  `abs(-2147483648) + 20` (`-2147483648` is `SINT32_MIN`). For the increase expression `%iv.inc = add nsw i32 %iv, 1`, it will not generate poison value(no overflow happens for the loop before transformation). 
> > > After LFTR, we use `%iv2` as loop iteration variable, `%iv2` start-value is -2, so to iterates times `loop_count` (21 bigger than SINT32_MAX), expression `[[IV2_INC]] = add nsw i32 [[IV2]], 1` will surely overflow, thus we will get poison value. 
> > > But after the change, no `nsw` anymore, so no poison value, we will do wrap to reach  `loop_count` times iterations. 
> > > Could you please help to point out where am I wrong? 
> > Either the test changes are regressions or this change is a bugfix (and LFTR was incorrect before).
> Ah yes... LFTR does have a known issue where nowrap flags are not always stripped correctly when switching to a dynamically dead IV. It indeed looks like the drop of nsw here is correct. What isn't clear to me though is why it happens: As I understand this patch, it is about making use of nsw/nuw information on addrecs even if the start is non-constant, so I would naively assume that we should only be inferring more flags with this change, not less.
The difference happens when we get SCEV for `%iv2.inc = add nuw nsw i32 %iv2, 1`.
Firstly, we get SCEV for PHI `%iv2 = phi i32 [ -2, %entry ], [ %iv2.inc, %always_taken ]`, it is `{-2,+,1}<nuw><nsw><%for.cond>`
When we try to get SCEV for `%iv2.inc = add nuw nsw i32 %iv2, 1`, it is `getAddExpr` with Ops `{1, {-2,+,1}<nuw><nsw><%for.cond>}`

We get a different result in `StrengthenNoWrapFlags` called by  `getAddExpr`.

```
    // (A <opcode> C) --> (A <opcode> C)<nsw> if the op doesn't sign overflow.
    if (!(SignOrUnsignWrap & SCEV::FlagNSW)) {
      auto NSWRegion = ConstantRange::makeGuaranteedNoWrapRegion(
          Opcode, C, OBO::NoSignedWrap);
      if (NSWRegion.contains(SE->getSignedRange(Ops[1])))
        Flags = ScalarEvolution::setFlags(Flags, SCEV::FlagNSW);
    }

    // (A <opcode> C) --> (A <opcode> C)<nuw> if the op doesn't unsign overflow.
    if (!(SignOrUnsignWrap & SCEV::FlagNUW)) {
      auto NUWRegion = ConstantRange::makeGuaranteedNoWrapRegion(
          Opcode, C, OBO::NoUnsignedWrap);
      if (NUWRegion.contains(SE->getUnsignedRange(Ops[1])))
        Flags = ScalarEvolution::setFlags(Flags, SCEV::FlagNUW);
    }
```

For `{-2,+,1}<nuw><nsw><%for.cond>`, with this patch's fixing, now the SignedRange is `[-2,-2147483629)`, the unsigned range is `[-2,0)`, so new range is not contained inside MGNWR, flag is 0.

Before the change, SignedRange is `[-2,0)`, unsigned range is `[-2,0)`. Signed Range is contained in Signed type MGNWR ([-2147483648,2147483647)),  flag is `SCEV::FlagNSW`. But Seems SignedRange is quite conservative?


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

https://reviews.llvm.org/D71690