Do not short circuit hoistIVInc when recomputation of poison flags is needed.

[resistor]

Summary

Fixes https://github.com/llvm/llvm-project/issues/59777

[nikic]

llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp

1028–1029

We can't just fall through to the code below if RecomputePoisonFlags is set. This both does more than we need, and I believe it will produce broken IR in some cases, because it may end up moving the instruction down, making it no longer dominate other users.

I believe what you'd want to do is extract the poison flag handling code below and then call it in this short-circuit code path as well.

llvm/test/Transforms/IndVarSimplify/iv-poison.ll

5

Include a reference to PR59777 here (e.g. in a comment).

Instructions in tests should always be named, you can achieve this by running the test through -passes=instnamer.

[resistor]

Extract poison fixup logic to a helper and apply it in the short-circuit case without
going through the rest of the hoisting process.

[nikic]

llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp

1044

nit: Should not be on one line.

1073–1074

Same

llvm/test/Transforms/IndVarSimplify/iv-poison.ll

5

Not addressed

[nikic]

(For the nits, otherwise fine)

[resistor]

Address review feedback.

[nikic]

LGTM. This will be a minor compile-time regression, but it's a correctness fix...

[resistor]

Fix improperly updated testcase.

llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp

for (auto *InsertPtGuard : InsertPointGuards)
     if (InsertPtGuard->GetInsertPoint() == It)
       InsertPtGuard->SetInsertPoint(NewInsertPt);
 }
 
 /// hoistStep - Attempt to hoist a simple IV increment above InsertPos to make
 /// it available to other uses in this loop. Recursively hoist any operands,
 /// until we reach a value that dominates InsertPos.
 bool SCEVExpander::hoistIVInc(Instruction *IncV, Instruction *InsertPos,
                               bool RecomputePoisonFlags) {
-  if (SE.DT.dominates(IncV, InsertPos))
-      return true;
+  auto FixupPoisonFlags = [this](Instruction *I) {
+    // Drop flags that are potentially inferred from old context and infer flags
+    // in new context.
+    I->dropPoisonGeneratingFlags();
+    if (auto *OBO = dyn_cast<OverflowingBinaryOperator>(I))
+      if (auto Flags = SE.getStrengthenedNoWrapFlagsFromBinOp(OBO)) {
+        auto *BO = cast<BinaryOperator>(I);
+        BO->setHasNoUnsignedWrap(
+            ScalarEvolution::maskFlags(*Flags, SCEV::FlagNUW) == SCEV::FlagNUW);
+        BO->setHasNoSignedWrap(
+            ScalarEvolution::maskFlags(*Flags, SCEV::FlagNSW) == SCEV::FlagNSW);
+      }
+  };
+
+  if (SE.DT.dominates(IncV, InsertPos)) {
+    if (RecomputePoisonFlags)
+      FixupPoisonFlags(IncV);
+  }
 
   // InsertPos must itself dominate IncV so that IncV's new position satisfies
   // its existing users.
   if (isa<PHINode>(InsertPos) ||
       !SE.DT.dominates(InsertPos->getParent(), IncV->getParent()))
     return false;
 
   if (!SE.LI.movementPreservesLCSSAForm(IncV, InsertPos))
for(;;) {
     IVIncs.push_back(IncV);
     IncV = Oper;
     if (SE.DT.dominates(IncV, InsertPos))
       break;
   }
   for (Instruction *I : llvm::reverse(IVIncs)) {
     fixupInsertPoints(I);
     I->moveBefore(InsertPos);
-    if (!RecomputePoisonFlags)
-      continue;
-    // Drop flags that are potentially inferred from old context and infer flags
-    // in new context.
-    I->dropPoisonGeneratingFlags();
-    if (auto *OBO = dyn_cast<OverflowingBinaryOperator>(I))
-      if (auto Flags = SE.getStrengthenedNoWrapFlagsFromBinOp(OBO)) {
-        auto *BO = cast<BinaryOperator>(I);
-        BO->setHasNoUnsignedWrap(
-            ScalarEvolution::maskFlags(*Flags, SCEV::FlagNUW) == SCEV::FlagNUW);
-        BO->setHasNoSignedWrap(
-            ScalarEvolution::maskFlags(*Flags, SCEV::FlagNSW) == SCEV::FlagNSW);
-      }
+    if (RecomputePoisonFlags)
+      FixupPoisonFlags(I);
   }
   return true;
 }
 
 /// Determine if this cyclic phi is in a form that would have been generated by
 /// LSR. We don't care if the phi was actually expanded in this pass, as long
 /// as it is in a low-cost form, for example, no implied multiplication. This
 /// should match any patterns generated by getAddRecExprPHILiterally and

llvm/test/Transforms/IndVarSimplify/iv-poison.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -passes=indvars -S < %s | FileCheck %s
+
+; PR59777
+define i2 @iv_hoist_nsw_poison(i2 %arg) {
+; CHECK-LABEL: @iv_hoist_nsw_poison(
+; CHECK-NEXT:  bb:
+; CHECK-NEXT:    br label [[BB1:%.*]]
+; CHECK:       bb1:
+; CHECK-NEXT:    [[DOT07:%.*]] = phi i2 [ 1, [[BB:%.*]] ], [ [[I:%.*]], [[BB1]] ]
+; CHECK-NEXT:    [[I]] = add nuw i2 [[DOT07]], 1
+; CHECK-NEXT:    [[DOTNOT_NOT:%.*]] = icmp ult i2 1, [[ARG:%.*]]
+; CHECK-NEXT:    br i1 [[DOTNOT_NOT]], label [[COMMON_RET:%.*]], label [[BB1]]
+; CHECK:       common.ret:
+; CHECK-NEXT:    [[I2_LCSSA:%.*]] = phi i2 [ [[I]], [[BB1]] ]
+; CHECK-NEXT:    ret i2 [[I2_LCSSA]]
+;
+bb:
+  br label %bb1
+
+bb1:                                              ; preds = %bb1, %bb
+  %.07 = phi i2 [ 1, %bb ], [ %i, %bb1 ]
+  %.0 = phi i2 [ 1, %bb ], [ %i2, %bb1 ]
+  %i = add nsw i2 %.07, 1
+  %i2 = add i2 %.0, 1
+  %.not.not = icmp ult i2 %.07, %arg
+  br i1 %.not.not, label %common.ret, label %bb1
+
+common.ret:                                       ; preds = %bb1
+  ret i2 %i2
+}