[PATCH] D95286: [LSR] Drop potentially invalid nowrap flags when switching to post-inc IV (PR46943)

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:1447
+    if (isa<OverflowingBinaryOperator>(Result)) {
+      if (auto *I = dyn_cast<Instruction>(Result)) {
+        if (!S->hasNoUnsignedWrap())
----------------
reames wrote:
> if (auto *op = dyn_cast<OBO>...)
> 
> e.g. you don't need the instruction, you can use the accessors on OBO.  
OBO can generally also be a constant expression, which is why it's not allowed to set nowrap flags directly on it. We need to go through Instruction to modify the flags.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:1451
+        if (!S->hasNoSignedWrap())
+          I->setHasNoSignedWrap(false);
+      }
----------------
reames wrote:
> Any reason not to just copy the SCEV flags?  Inferring stronger flags should be legal here.
I don't think that's quite correct without additional checks. We're checking the flags on the post-inc addrec, which don't make any statement about overflow on the first iteration. If you have something like pre-inc `{255,+,1}` and `{0,+,1}` post-inc, then the latter would be nuw (assuming appropriate BE count), while the former would not be. The add can't be nuw in that case, due to the overflow on the first iteration.


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/X86/sibling-loops.ll:20
 ; CHECK-NEXT:    tail call void @goo(i64 [[I_0]], i64 [[I_0]])
-; CHECK-NEXT:    [[INC]] = add nuw nsw i64 [[I_0]], 1
+; CHECK-NEXT:    [[INC]] = add nuw i64 [[I_0]], 1
 ; CHECK-NEXT:    [[T0:%.*]] = load i64, i64* @cond, align 8
----------------
reames wrote:
> Given the nsw is present in the source, SCEV should know this is nsw.  Any idea why it doesn't?
The `%inc` IV doesn't seem to ever be branched on, so there's no guarantee that %inc being poison would result in undefined behavior. Thus SCEV can't transfer poison flags from IR. There are some additional cases we could transfer using D92739 (for branches in non-latch exits), but I don't think that would help this case either.


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

https://reviews.llvm.org/D95286