[llvm] 915bcb0 - [LSR] Style cleanup for code recently added in D132443

[Philip Reames via llvm-commits]

Author: Philip Reames
Date: 2023-01-20T12:45:37-08:00
New Revision: 915bcb062964d783fae0f509f3f5d4f7b770f2e2

URL: https://github.com/llvm/llvm-project/commit/915bcb062964d783fae0f509f3f5d4f7b770f2e2
DIFF: https://github.com/llvm/llvm-project/commit/915bcb062964d783fae0f509f3f5d4f7b770f2e2.diff

LOG: [LSR] Style cleanup for code recently added in D132443

Also add FIXMEs which highlight correctness bugs in this recently added off by default option.  These have also been raised on the original review.

Added: 
    

Modified: 
    llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
index f04420bbf44a..3818ef2a02bf 100644
--- a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
@@ -6607,7 +6607,7 @@ static llvm::PHINode *GetInductionVariable(const Loop &L, ScalarEvolution &SE,
   return nullptr;
 }
 
-static std::optional<std::pair<PHINode *, std::pair<PHINode *, const SCEV *>>>
+static std::optional<std::tuple<PHINode *, PHINode *, const SCEV *>>
 canFoldTermCondOfLoop(Loop *L, ScalarEvolution &SE, DominatorTree &DT,
                       const LoopInfo &LI) {
   if (!L->isInnermost()) {
@@ -6689,9 +6689,10 @@ canFoldTermCondOfLoop(Loop *L, ScalarEvolution &SE, DominatorTree &DT,
     return VToPN && VToTermCond;
   };
 
-  // For `IsToHelpFold`, other IV that is an affine AddRec will be sufficient to
-  // replace the terminating condition
-  auto IsToHelpFold = [&](PHINode &PN) -> std::pair<bool, const SCEV *> {
+  // If this is an IV which we could replace the terminating condition, return
+  // the final value of the alternative IV on the last iteration.
+  auto getAlternateIVEnd = [&](PHINode &PN) -> const SCEV * {
+    // FIXME: This does not properly account for overflow.
     const SCEVAddRecExpr *AddRec = cast<SCEVAddRecExpr>(SE.getSCEV(&PN));
     const SCEV *BECount = SE.getBackedgeTakenCount(L);
     const SCEV *TermValueS = SE.getAddExpr(
@@ -6709,9 +6710,9 @@ canFoldTermCondOfLoop(Loop *L, ScalarEvolution &SE, DominatorTree &DT,
       LLVM_DEBUG(
           dbgs() << "Is not safe to expand terminating value for phi node" << PN
                  << "\n");
-      return {false, nullptr};
+      return nullptr;
     }
-    return {true, TermValueS};
+    return TermValueS;
   };
 
   PHINode *ToFold = nullptr;
@@ -6737,9 +6738,9 @@ canFoldTermCondOfLoop(Loop *L, ScalarEvolution &SE, DominatorTree &DT,
 
     if (IsToFold(PN))
       ToFold = &PN;
-    else if (auto P = IsToHelpFold(PN); P.first) {
+    else if (auto P = getAlternateIVEnd(PN)) {
       ToHelpFold = &PN;
-      TermValueS = P.second;
+      TermValueS = P;
     }
   }
 
@@ -6756,7 +6757,7 @@ canFoldTermCondOfLoop(Loop *L, ScalarEvolution &SE, DominatorTree &DT,
 
   if (!ToFold || !ToHelpFold)
     return std::nullopt;
-  return {{ToFold, {ToHelpFold, TermValueS}}};
+  return std::make_tuple(ToFold, ToHelpFold, TermValueS);
 }
 
 static bool ReduceLoopStrength(Loop *L, IVUsers &IU, ScalarEvolution &SE,
@@ -6818,17 +6819,15 @@ static bool ReduceLoopStrength(Loop *L, IVUsers &IU, ScalarEvolution &SE,
   }
 
   if (AllowTerminatingConditionFoldingAfterLSR) {
-    auto CanFoldTerminatingCondition = canFoldTermCondOfLoop(L, SE, DT, LI);
-    if (CanFoldTerminatingCondition) {
+    if (auto Opt = canFoldTermCondOfLoop(L, SE, DT, LI)) {
+      auto [ToFold, ToHelpFold, TermValueS] = *Opt;
+
       Changed = true;
       NumTermFold++;
 
       BasicBlock *LoopPreheader = L->getLoopPreheader();
       BasicBlock *LoopLatch = L->getLoopLatch();
 
-      PHINode *ToFold = CanFoldTerminatingCondition->first;
-      PHINode *ToHelpFold = CanFoldTerminatingCondition->second.first;
-
       (void)ToFold;
       LLVM_DEBUG(dbgs() << "To fold phi-node:\n"
                         << *ToFold << "\n"
@@ -6843,12 +6842,10 @@ static bool ReduceLoopStrength(Loop *L, IVUsers &IU, ScalarEvolution &SE,
       SCEVExpander Expander(SE, DL, "lsr_fold_term_cond");
       SCEVExpanderCleaner ExpCleaner(Expander);
 
-      // Create new terminating value at loop header
-      const SCEV *TermValueS = CanFoldTerminatingCondition->second.second;
-      assert(
-          Expander.isSafeToExpand(TermValueS) &&
-          "Terminating value was checked safe in canFoldTerminatingCondition");
+      assert(Expander.isSafeToExpand(TermValueS) &&
+             "Terminating value was checked safe in canFoldTerminatingCondition");
 
+      // Create new terminating value at loop header
       Value *TermValue = Expander.expandCodeFor(TermValueS, ToHelpFold->getType(),
                                                 LoopPreheader->getTerminator());
 
@@ -6861,6 +6858,8 @@ static bool ReduceLoopStrength(Loop *L, IVUsers &IU, ScalarEvolution &SE,
       BranchInst *BI = cast<BranchInst>(LoopLatch->getTerminator());
       ICmpInst *OldTermCond = cast<ICmpInst>(BI->getCondition());
       IRBuilder<> LatchBuilder(LoopLatch->getTerminator());
+      // FIXME: We are adding a use of an IV here without account for poison safety.
+      // This is incorrect.
       Value *NewTermCond = LatchBuilder.CreateICmp(
           OldTermCond->getPredicate(), LoopValue, TermValue,
           "lsr_fold_term_cond.replaced_term_cond");