[PATCH] D60935: [IndVarSimplify] Fixup nowrap flags during LFTR when moving to post-inc (PR31181)

[Nikita Popov via Phabricator via llvm-commits]

nikic marked an inline comment as done.
nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/IndVarSimplify.cpp:2383
+  // instruction, while SCEV has to explicitly prove the post-inc nowrap flags.
+  // TODO: This handling may be inaccurate for one case: If we switch to a
+  // dynamically dead IV that wraps on the first loop iteration only, which is
----------------
reames wrote:
> I don't think this is possible given hasConcreteDef.  It ensures the value on the first iteration is non-poison.  
hasConcreteDef() ensures that it's not poison on entry into the loop, but doesn't make sure that it's non-poison after the first iteration. The post-inc addrec only applies after the first iteration. Here is an example (CHECK lines are with this patch):

```
define i32 @switch_to_different_iv_first_poison(i32* %ptr, i1 %always_false) {
; CHECK-LABEL: @switch_to_different_iv_first_poison(
; CHECK-NEXT:  entry:
; CHECK-NEXT:    br label [[FOR_COND:%.*]]
; CHECK:       for.cond:
; CHECK-NEXT:    [[IV2:%.*]] = phi i32 [ -1, [[ENTRY:%.*]] ], [ [[IV2_INC:%.*]], [[ALWAYS_TAKEN:%.*]] ]
; CHECK-NEXT:    [[IV:%.*]] = phi i32 [ -2147483648, [[ENTRY]] ], [ [[IV_INC:%.*]], [[ALWAYS_TAKEN]] ]
; CHECK-NEXT:    store i32 [[IV]], i32* [[PTR:%.*]]
; CHECK-NEXT:    br i1 [[ALWAYS_FALSE:%.*]], label [[NEVER_TAKEN:%.*]], label [[ALWAYS_TAKEN]]
; CHECK:       never_taken:
; CHECK-NEXT:    store volatile i32 [[IV2]], i32* [[PTR]]
; CHECK-NEXT:    br label [[ALWAYS_TAKEN]]
; CHECK:       always_taken:
; CHECK-NEXT:    [[IV2_INC]] = add nuw nsw i32 [[IV2]], 1
; CHECK-NEXT:    [[IV_INC]] = add nsw i32 [[IV]], 1
; CHECK-NEXT:    [[EXITCOND:%.*]] = icmp ne i32 [[IV2_INC]], -2147483628
; CHECK-NEXT:    br i1 [[EXITCOND]], label [[FOR_COND]], label [[FOR_END:%.*]]
; CHECK:       for.end:
; CHECK-NEXT:    ret i32 0
;
entry:
  br label %for.cond

for.cond:
  %iv2 = phi i32 [ -1, %entry ], [ %iv2.inc, %always_taken ]
  %iv = phi i32 [ -2147483648, %entry ], [ %iv.inc, %always_taken ]
  store i32 %iv, i32* %ptr
  br i1 %always_false, label %never_taken, label %always_taken

never_taken:
  store volatile i32 %iv2, i32* %ptr
  br label %always_taken

always_taken:
  %iv2.inc = add nuw nsw i32 %iv2, 1
  %iv.inc = add nsw i32 %iv, 1
  %cmp = icmp slt i32 %iv, 20
  br i1 %cmp, label %for.cond, label %for.end

for.end:
  ret i32 0
}
```

As you can see, we switched to `%iv2.inc`, but did not drop any nowrap flags.

Unfortunately I found out that there seems to be another issue that affects not just the first iteration. Here is a small variation on the above test case, starting from -2 instead of -1:

```
define i32 @switch_to_different_iv_second_poison(i32* %ptr, i1 %always_false) {
; CHECK-LABEL: @switch_to_different_iv_second_poison(
; CHECK-NEXT:  entry:
; CHECK-NEXT:    br label [[FOR_COND:%.*]]
; CHECK:       for.cond:
; CHECK-NEXT:    [[IV2:%.*]] = phi i32 [ -2, [[ENTRY:%.*]] ], [ [[IV2_INC:%.*]], [[ALWAYS_TAKEN:%.*]] ]
; CHECK-NEXT:    [[IV:%.*]] = phi i32 [ -2147483648, [[ENTRY]] ], [ [[IV_INC:%.*]], [[ALWAYS_TAKEN]] ]
; CHECK-NEXT:    store i32 [[IV]], i32* [[PTR:%.*]]
; CHECK-NEXT:    br i1 [[ALWAYS_FALSE:%.*]], label [[NEVER_TAKEN:%.*]], label [[ALWAYS_TAKEN]]
; CHECK:       never_taken:
; CHECK-NEXT:    store volatile i32 [[IV2]], i32* [[PTR]]
; CHECK-NEXT:    br label [[ALWAYS_TAKEN]]
; CHECK:       always_taken:
; CHECK-NEXT:    [[IV2_INC]] = add nsw i32 [[IV2]], 1
; CHECK-NEXT:    [[IV_INC]] = add nsw i32 [[IV]], 1
; CHECK-NEXT:    [[EXITCOND:%.*]] = icmp ne i32 [[IV2_INC]], -2147483629
; CHECK-NEXT:    br i1 [[EXITCOND]], label [[FOR_COND]], label [[FOR_END:%.*]]
; CHECK:       for.end:
; CHECK-NEXT:    ret i32 0
;
entry:
  br label %for.cond

for.cond:
  %iv2 = phi i32 [ -2, %entry ], [ %iv2.inc, %always_taken ]
  %iv = phi i32 [ -2147483648, %entry ], [ %iv.inc, %always_taken ]
  store i32 %iv, i32* %ptr
  br i1 %always_false, label %never_taken, label %always_taken

never_taken:
  store volatile i32 %iv2, i32* %ptr
  br label %always_taken

always_taken:
  %iv2.inc = add nuw nsw i32 %iv2, 1
  %iv.inc = add nsw i32 %iv, 1
  %cmp = icmp slt i32 %iv, 20
  br i1 %cmp, label %for.cond, label %for.end

for.end:
  ret i32 0
}
```

In this case the nuw flag is correctly dropped, but the nsw flag stays. I think this might be related to the fact that for the pre-inc addrec nowrap flags from IR are adopted unconditionally, and we may end up inferring something incorrect for the post-inc addrec from that (e.g. because it over-constrains ranges). I get the impression that SCEV might just not be prepared to deal with dynamically dead IVs.


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

https://reviews.llvm.org/D60935