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

Thu Dec 19 12:45:00 PST 2019

nikic added inline comments.

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

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

https://reviews.llvm.org/D71690