Extend support for creating SCEVs iteratively

krystian-andrzejewski · igcbot · commit ab90d29feb86 · 2024-06-24T14:43:50.000+02:00
This change is to avoid stack overflow issues related to creating scalar evolution expressions. The previous solution can lead to unintended scenarios when building an expression chain requires to use a recursive approach extensively. Moreover, there is an improvement in the quality of these expressions because of creating a final expression of the phi instruction only in the recursive function.
diff --git a/external/llvm/releases/14.0.0/patches_external/0002-ScalarEvolutionStackOverflowWindows.patch b/external/llvm/releases/14.0.0/patches_external/0002-ScalarEvolutionStackOverflowWindows.patch
@@ -1,6 +1,6 @@
 /*========================== begin_copyright_notice ============================
 
-Copyright (C) 2023 Intel Corporation
+Copyright (C) 2023-2024 Intel Corporation
 
 SPDX-License-Identifier: MIT
 
@@ -14,84 +14,175 @@ SPDX-License-Identifier: Apache-2.0 with LLVM-exception
 
 ============================= end_copyright_notice ===========================*/
 
-# TODO: Once upstreamed, update with LLORG revision & adjust per community review
-
-diff -Naur "a/llvm/include/llvm/Analysis/ScalarEvolution.h" "b/llvm/include/llvm/Analysis/ScalarEvolution.h"
---- "a/llvm/include/llvm/Analysis/ScalarEvolution.h"	2023-09-27 19:27:08.174510600 +0200
-+++ "b/llvm/include/llvm/Analysis/ScalarEvolution.h"	2023-09-27 18:41:11.301171800 +0200
-@@ -1292,6 +1292,9 @@
-   /// Mark predicate values currently being processed by isImpliedCond.
-   SmallPtrSet<const Value *, 6> PendingLoopPredicates;
- 
-+  /// Set of Phis processed in createSCEVIter.
-+  SmallPtrSet<const PHINode*, 6> PendingPhiSCEVIter;
+diff --git a/llvm/include/llvm/Analysis/ScalarEvolution.h b/llvm/include/llvm/Analysis/ScalarEvolution.h
+index 51558a37b..a2069980a 100644
+--- a/llvm/include/llvm/Analysis/ScalarEvolution.h
++++ b/llvm/include/llvm/Analysis/ScalarEvolution.h
+@@ -1613,6 +1613,9 @@ private:
+   /// Provide the special handling we need to analyze PHI SCEVs.
+   const SCEV *createNodeForPHI(PHINode *PN);
+ 
++  /// Helper function to get a single start and back edge value from a phi node.
++  std::pair<Value*, Value*> getStartAndBackEdgeValues(PHINode *PN);
 +
-   /// Mark SCEVUnknown Phis currently being processed by getRangeRef.
-   SmallPtrSet<const PHINode *, 6> PendingPhiRanges;
+   /// Helper function called from createNodeForPHI.
+   const SCEV *createAddRecFromPHI(PHINode *PN);
+ 
+diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
+index fc9517ebb..facfe1c7d 100644
+--- a/llvm/lib/Analysis/ScalarEvolution.cpp
++++ b/llvm/lib/Analysis/ScalarEvolution.cpp
+@@ -5529,10 +5529,11 @@ const SCEV *ScalarEvolution::createSimpleAffineAddRec(PHINode *PN,
+   return PHISCEV;
+ }
+ 
+-const SCEV *ScalarEvolution::createAddRecFromPHI(PHINode *PN) {
++std::pair<Value *, Value *>
++ScalarEvolution::getStartAndBackEdgeValues(PHINode *PN) {
+   const Loop *L = LI.getLoopFor(PN->getParent());
+   if (!L || L->getHeader() != PN->getParent())
+-    return nullptr;
++    return {};
+ 
+   // The loop may have multiple entrances or multiple exits; we can analyze
+   // this phi as an addrec if it has a unique entry value and a unique
+@@ -5555,10 +5556,18 @@ const SCEV *ScalarEvolution::createAddRecFromPHI(PHINode *PN) {
+     }
+   }
+   if (!BEValueV || !StartValueV)
+-    return nullptr;
++    return {};
++  return {StartValueV, BEValueV};
++}
  
-diff -Naur "a/llvm/lib/Analysis/ScalarEvolution.cpp" "b/llvm/lib/Analysis/ScalarEvolution.cpp"
---- "a/llvm/lib/Analysis/ScalarEvolution.cpp"	2023-09-27 19:27:08.189098000 +0200
-+++ "b/llvm/lib/Analysis/ScalarEvolution.cpp"	2023-09-27 19:05:40.601713600 +0200
-@@ -7004,6 +7004,13 @@
+-  assert(ValueExprMap.find_as(PN) == ValueExprMap.end() &&
+-         "PHI node already processed?");
++const SCEV *ScalarEvolution::createAddRecFromPHI(PHINode *PN) {
++  // The loop may have multiple entrances or multiple exits; we can analyze
++  // this phi as an addrec if it has a unique entry value and a unique
++  // backedge value.
++  std::pair<Value *, Value *> StartAndBEValueV = getStartAndBackEdgeValues(PN);
++  Value *BEValueV = StartAndBEValueV.second, *StartValueV = StartAndBEValueV.first;
++  if (!BEValueV || !StartValueV)
++    return nullptr;
+ 
+   // First, try to find AddRec expression without creating a fictituos symbolic
+   // value for PN.
+@@ -5572,6 +5581,7 @@ const SCEV *ScalarEvolution::createAddRecFromPHI(PHINode *PN) {
+   // Using this symbolic name for the PHI, analyze the value coming around
+   // the back-edge.
+   const SCEV *BEValue = getSCEV(BEValueV);
++  const Loop *L = LI.getLoopFor(PN->getParent());
+ 
+   // NOTE: If BEValue is loop invariant, we know that the PHI node just
+   // has a special value for the first iteration of the loop.
+@@ -5772,8 +5782,15 @@ static bool IsAvailableOnEntry(const Loop *L, DominatorTree &DT, const SCEV *S,
+ // Try to match a control flow sequence that branches out at BI and merges back
+ // at Merge into a "C ? LHS : RHS" select pattern.  Return true on a successful
+ // match.
+-static bool BrPHIToSelect(DominatorTree &DT, BranchInst *BI, PHINode *Merge,
+-                          Value *&C, Value *&LHS, Value *&RHS) {
++static bool BrPHIToSelect(DominatorTree &DT, PHINode *Merge, Value *&C,
++                          Value *&LHS, Value *&RHS) {
++
++  BasicBlock *IDom = DT[Merge->getParent()]->getIDom()->getBlock();
++  assert(IDom && "At least the entry block should dominate PN");
++
++  auto *BI = dyn_cast<BranchInst>(IDom->getTerminator());
++  if (!BI || !BI->isConditional())
++    return false;
+   C = BI->getCondition();
+ 
+   BasicBlockEdge LeftEdge(BI->getParent(), BI->getSuccessor(0));
+@@ -5802,6 +5819,11 @@ static bool BrPHIToSelect(DominatorTree &DT, BranchInst *BI, PHINode *Merge,
+   return false;
+ }
+ 
++static bool BrPHIToSelect(DominatorTree& DT, PHINode* Merge) {
++  Value *Cond = nullptr, *LHS = nullptr, *RHS = nullptr;
++  return BrPHIToSelect(DT, Merge, Cond, LHS, RHS);
++}
++
+ const SCEV *ScalarEvolution::createNodeFromSelectLikePHI(PHINode *PN) {
+   auto IsReachable =
+       [&](BasicBlock *BB) { return DT.isReachableFromEntry(BB); };
+@@ -5824,18 +5846,16 @@ const SCEV *ScalarEvolution::createNodeFromSelectLikePHI(PHINode *PN) {
+     //  V = phi [ %x, %left ], [ %y, %right ]
+     //
+     // as "select %cond, %x, %y"
+-
+-    BasicBlock *IDom = DT[PN->getParent()]->getIDom()->getBlock();
+-    assert(IDom && "At least the entry block should dominate PN");
+-
+-    auto *BI = dyn_cast<BranchInst>(IDom->getTerminator());
+     Value *Cond = nullptr, *LHS = nullptr, *RHS = nullptr;
+ 
+-    if (BI && BI->isConditional() &&
+-        BrPHIToSelect(DT, BI, PN, Cond, LHS, RHS) &&
++    // do not run over cycled PHIs while processing createNodeFromSelectLikePHI
++    const SCEV* S = nullptr;
++    if (BrPHIToSelect(DT, PN, Cond, LHS, RHS) &&
+         IsAvailableOnEntry(L, DT, getSCEV(LHS), PN->getParent()) &&
+-        IsAvailableOnEntry(L, DT, getSCEV(RHS), PN->getParent()))
+-      return createNodeForSelectOrPHI(PN, Cond, LHS, RHS);
++        IsAvailableOnEntry(L, DT, getSCEV(RHS), PN->getParent())) {
++        S = createNodeForSelectOrPHI(PN, Cond, LHS, RHS);
++    }
++    return S;
+   }
+ 
+   return nullptr;
+@@ -7004,6 +7024,7 @@ const SCEV *ScalarEvolution::createSCEVIter(Value *V) {
  
    Stack.emplace_back(V, true);
    Stack.emplace_back(V, false);
-+
-+  if (isa<PHINode>(V) &&
-+      !PendingPhiSCEVIter.insert(cast<PHINode>(V)).second) {
-+    // cycled back to createSCEVIter while processing this Phi, bail
-+    return getUnknown(V);
-+  }
 +
    while (!Stack.empty()) {
      auto E = Stack.pop_back_val();
      Value *CurV = E.getPointer();
-@@ -7016,6 +7023,10 @@
-     // If all operands have been visited already, create the SCEV.
-     if (E.getInt()) {
-       CreatedSCEV = createSCEV(CurV);
-+    } else if (isa<PHINode>(CurV) && 
-+      PendingPhiSCEVIter.count(cast<PHINode>(CurV)) > 0) {
-+      // cycled Phi, try to get its SCEV
-+      CreatedSCEV = createSCEV(CurV);
-     } else {
-       // Otherwise get the operands we need to create SCEV's for before creating
-       // the SCEV for CurV. If the SCEV for CurV can be constructed trivially,
-@@ -7025,12 +7036,17 @@
- 
-     if (CreatedSCEV) {
+@@ -7023,14 +7044,15 @@ const SCEV *ScalarEvolution::createSCEVIter(Value *V) {
+       CreatedSCEV = getOperandsToCreate(CurV, Ops);
+     }
+ 
+-    if (CreatedSCEV) {
++    if (CreatedSCEV)
        insertValueToMap(CurV, CreatedSCEV);
-+      if (isa<PHINode>(CurV))
-+          PendingPhiSCEVIter.erase(cast<PHINode>(CurV));
-     } else {
+-    } else {
++    else {
        // Queue CurV for SCEV creation, followed by its's operands which need to
        // be constructed first.
        Stack.emplace_back(CurV, true);
 -      for (Value *Op : Ops)
-+      if (isa<PHINode>(CurV))
-+          PendingPhiSCEVIter.insert(cast<PHINode>(CurV));
 +      for (Value *Op : Ops) {
          Stack.emplace_back(Op, false);
 +      }
      }
    }
  
-@@ -7153,7 +7169,8 @@
+@@ -7152,9 +7174,24 @@ ScalarEvolution::getOperandsToCreate(Value *V, SmallVectorImpl<Value *> &Ops) {
+   case Instruction::IntToPtr:
      return getUnknown(V);
  
-   case Instruction::PHI:
+-  case Instruction::PHI:
 -    // Keep constructing SCEVs' for phis recursively for now.
-+    for (auto& Op : cast<PHINode>(U)->operands())
-+      Ops.push_back(Op);
++  case Instruction::PHI: {
++    PHINode *PN = cast<PHINode>(U);
++
++    if (PN->getNumIncomingValues() == 2) {
++      std::pair<Value *, Value *> StartAndBEValueV =
++          getStartAndBackEdgeValues(PN);
++      if (StartAndBEValueV.first && StartAndBEValueV.second)
++        // resolve recursively - may create cycles
++        return nullptr;
++      else if (BrPHIToSelect(DT, PN))
++        // resolve iteratively - does not create cycles
++        for (auto &Op : PN->operands())
++          Ops.push_back(Op);
++    }
++
++    // resolve iteratively - does not create SCEV cycles (trivially resolved)
      return nullptr;
++  }
  
    case Instruction::Select: {
-@@ -13405,6 +13422,8 @@
-         auto ValueIt = ValueExprMap.find_as(ValueAndOffset.first);
-         if (ValueIt != ValueExprMap.end())
-           ValueExprMap.erase(ValueIt);
-+        if (isa<PHINode>(ValueAndOffset.first))
-+          PendingPhiSCEVIter.erase(cast<PHINode>(ValueAndOffset.first));
-       }
-     }
-     ExprValueMap.erase(ExprIt);
+     // Check if U is a select that can be simplified to a SCEVUnknown.
diff --git a/external/llvm/releases/14.0.0/patches_external/0003-ScalarEvolutionStackOverflowWindows.patch b/external/llvm/releases/14.0.0/patches_external/0003-ScalarEvolutionStackOverflowWindows.patch
@@ -1,6 +1,6 @@
 /*========================== begin_copyright_notice ============================
 
-Copyright (C) 2023 Intel Corporation
+Copyright (C) 2023-2024 Intel Corporation
 
 SPDX-License-Identifier: MIT
 
@@ -16,43 +16,95 @@ SPDX-License-Identifier: Apache-2.0 with LLVM-exception
 
 # TODO: Once upstreamed, update with LLORG revision & adjust per community review
 
-diff -Naur "a/llvm/include/llvm/Analysis/ScalarEvolution.h" "b/llvm/include/llvm/Analysis/ScalarEvolution.h"
---- "a/llvm/include/llvm/Analysis/ScalarEvolution.h"	2023-12-19 12:51:55.043965800 +0100
-+++ "b/llvm/include/llvm/Analysis/ScalarEvolution.h"	2023-12-19 12:28:39.120139600 +0100
-@@ -1295,6 +1295,9 @@
-   /// Set of Phis processed in createSCEVIter.
-   SmallPtrSet<const PHINode*, 6> PendingPhiSCEVIter;
- 
-+  /// Set of Phis processed in createPhiNodeFromSelectLikePHI.
-+  SmallPtrSet<const PHINode*, 6> PendingPhiNodeFromSelectLikePHI;
-+
-   /// Mark SCEVUnknown Phis currently being processed by getRangeRef.
-   SmallPtrSet<const PHINode *, 6> PendingPhiRanges;
- 
-diff -Naur "a/llvm/lib/Analysis/ScalarEvolution.cpp" "b/llvm/lib/Analysis/ScalarEvolution.cpp"
---- "a/llvm/lib/Analysis/ScalarEvolution.cpp"	2023-12-19 12:52:55.207074600 +0100
-+++ "b/llvm/lib/Analysis/ScalarEvolution.cpp"	2023-12-19 12:37:39.188525900 +0100
-@@ -5836,11 +5836,18 @@
-     auto *BI = dyn_cast<BranchInst>(IDom->getTerminator());
-     Value *Cond = nullptr, *LHS = nullptr, *RHS = nullptr;
- 
--    if (BI && BI->isConditional() &&
--        BrPHIToSelect(DT, BI, PN, Cond, LHS, RHS) &&
--        IsAvailableOnEntry(L, DT, getSCEV(LHS), PN->getParent()) &&
--        IsAvailableOnEntry(L, DT, getSCEV(RHS), PN->getParent()))
--      return createNodeForSelectOrPHI(PN, Cond, LHS, RHS);
-+    // do not run over cycled PHIs while processing createNodeFromSelectLikePHI
-+    if (PendingPhiNodeFromSelectLikePHI.insert(PN).second) {
-+      const SCEV* S = nullptr;
-+      if (BI && BI->isConditional() &&
-+         BrPHIToSelect(DT, BI, PN, Cond, LHS, RHS) &&
-+         IsAvailableOnEntry(L, DT, getSCEV(LHS), PN->getParent()) &&
-+         IsAvailableOnEntry(L, DT, getSCEV(RHS), PN->getParent())) {
-+          S = createNodeForSelectOrPHI(PN, Cond, LHS, RHS);
-+      }
-+      PendingPhiNodeFromSelectLikePHI.erase(PN);
-+      return S;
-+    }
+diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
+index e765a4d54..518683c29 100644
+--- a/llvm/lib/Analysis/ScalarEvolution.cpp
++++ b/llvm/lib/Analysis/ScalarEvolution.cpp
+@@ -5576,12 +5576,14 @@ const SCEV *ScalarEvolution::createAddRecFromPHI(PHINode *PN) {
+ 
+   // First, try to find AddRec expression without creating a fictituos symbolic
+   // value for PN.
+-  if (auto *S = createSimpleAffineAddRec(PN, BEValueV, StartValueV))
+-    return S;
++  // if (auto *S = createSimpleAffineAddRec(PN, BEValueV, StartValueV))
++  //   return S;
+ 
+   // Handle PHI node value symbolically.
+-  const SCEV *SymbolicName = getUnknown(PN);
+-  insertValueToMap(PN, SymbolicName);
++  // const SCEV *SymbolicName = getUnknown(PN);
++  // insertValueToMap(PN, SymbolicName);
++  const SCEV *SymbolicName = getExistingSCEV(PN);
++  assert(SymbolicName);
+ 
+   // Using this symbolic name for the PHI, analyze the value coming around
+   // the back-edge.
+@@ -5789,6 +5791,8 @@ static bool IsAvailableOnEntry(const Loop *L, DominatorTree &DT, const SCEV *S,
+ // match.
+ static bool BrPHIToSelect(DominatorTree &DT, PHINode *Merge, Value *&C,
+                           Value *&LHS, Value *&RHS) {
++  if (Merge->getNumIncomingValues() != 2)
++    return false;
+ 
+   BasicBlock *IDom = DT[Merge->getParent()]->getIDom()->getBlock();
+   assert(IDom && "At least the entry block should dominate PN");
+@@ -7094,14 +7098,13 @@ const SCEV *ScalarEvolution::createSCEVIter(Value *V) {
+   using PointerTy = PointerIntPair<Value *, 1, bool>;
+   SmallVector<PointerTy> Stack;
+ 
+-  Stack.emplace_back(V, true);
+   Stack.emplace_back(V, false);
+ 
+   while (!Stack.empty()) {
+     auto E = Stack.pop_back_val();
+     Value *CurV = E.getPointer();
+ 
+-    if (getExistingSCEV(CurV))
++    if (getExistingSCEV(CurV) && !E.getInt())
+       continue;
+ 
+     SmallVector<Value *> Ops;
+@@ -7118,7 +7121,7 @@ const SCEV *ScalarEvolution::createSCEVIter(Value *V) {
+ 
+     if (CreatedSCEV)
+       insertValueToMap(CurV, CreatedSCEV);
+-    else {
++    if (!Ops.empty() || !CreatedSCEV) {
+       // Queue CurV for SCEV creation, followed by its's operands which need to
+       // be constructed first.
+       Stack.emplace_back(CurV, true);
+@@ -7248,21 +7251,19 @@ ScalarEvolution::getOperandsToCreate(Value *V, SmallVectorImpl<Value *> &Ops) {
+ 
+   case Instruction::PHI: {
+     PHINode *PN = cast<PHINode>(U);
+-
+-    if (PN->getNumIncomingValues() == 2) {
+-      std::pair<Value *, Value *> StartAndBEValueV =
+-          getStartAndBackEdgeValues(PN);
+-      if (StartAndBEValueV.first && StartAndBEValueV.second)
+-        // resolve recursively - may create cycles
+-        return nullptr;
+-      else if (BrPHIToSelect(DT, PN))
+-        // resolve iteratively - does not create cycles
+-        for (auto &Op : PN->operands())
+-          Ops.push_back(Op);
+-    }
+-
+-    // resolve iteratively - does not create SCEV cycles (trivially resolved)
+-    return nullptr;
++    // This phi's operands must be attached due to range computations even if it is redundant
++    // to determine its SCEV expression.
++    for (auto &Op : PN->operands())
++      Ops.push_back(Op);
++    if (BrPHIToSelect(DT, PN))
++      // This phi instruction might be classified as a phi-like-select expression.
++      // Any initial definition cannot be applied since this pattern doesn't support
++      // such a path as dedicated to addrec expressions.
++      return nullptr;
++    // This phi instruction is initially defined as undefined and can be replaced
++    // with an addrec expression if it meets requirements. Otherwise,
++    // this will be preserved.
++    return getUnknown(V);
    }
  
-   return nullptr;
+   case Instruction::Select: {
