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

[Sanjoy Das (Work Account) via Phabricator via llvm-commits]

sanjoy.google requested changes to this revision.
sanjoy.google added a comment.
This revision now requires changes to proceed.

This needs a specific test case.



================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5658
+    if (AddRec->hasNoUnsignedWrap()) {
+      APInt MinValue = APInt(BitWidth, 0);
+      if (SignHint == ScalarEvolution::HINT_RANGE_UNSIGNED)
----------------
This is the unsigned minimum value right?  If so please name it `UnsignedMinValue`.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5661
+        MinValue = getUnsignedRangeMin(AddRec->getStart());
+      else {
+        ConstantRange StartRange =
----------------
Can you comment on why you need the else block?


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


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

https://reviews.llvm.org/D71690