CycleInfo: Fix splitCriticalEdge

[petar-avramovic]

There are cases when cycle that contains both Pred and Succ is not found: Pred is in a smaller cycle that is contained in a larger cycle that also contains Succ, or Pred and Succ are in disjointed innermost cycles but there is a larger cycle that contains both.
Add efficient helper function that finds innermost cycle that contains both Pred and Succ if it exists.

[llvmbot]

@llvm/pr-subscribers-llvm-adt

Changes

There are cases when cycle that contains both Pred and Succ is not found: Pred is in a smaller cycle that is contained in a larger cycle that also contains Succ, or Pred and Succ are in disjointed innermost cycles but there is a larger cycle that contains both.
Add efficient helper function that finds innermost cycle that contains both Pred and Succ if it exists.

---

Full diff: https://github.com/llvm/llvm-project/pull/68584.diff

2 Files Affected:

• (modified) llvm/include/llvm/ADT/GenericCycleImpl.h (+38-10)
• (modified) llvm/include/llvm/ADT/GenericCycleInfo.h (+1)

diff --git a/llvm/include/llvm/ADT/GenericCycleImpl.h b/llvm/include/llvm/ADT/GenericCycleImpl.h
index 2adec725cd3bf7d..6f74d0825e1346d 100644
--- a/llvm/include/llvm/ADT/GenericCycleImpl.h
+++ b/llvm/include/llvm/ADT/GenericCycleImpl.h
@@ -368,16 +368,14 @@ void GenericCycleInfo<ContextT>::splitCriticalEdge(BlockT *Pred, BlockT *Succ,
                                                    BlockT *NewBlock) {
   // Edge Pred-Succ is replaced by edges Pred-NewBlock and NewBlock-Succ, all
   // cycles that had blocks Pred and Succ also get NewBlock.
-  CycleT *Cycle = this->getCycle(Pred);
-  if (Cycle && Cycle->contains(Succ)) {
-    while (Cycle) {
-      // FixMe: Appending NewBlock is fine as a set of blocks in a cycle. When
-      // printing cycle NewBlock is at the end of list but it should be in the
-      // middle to represent actual traversal of a cycle.
-      Cycle->appendBlock(NewBlock);
-      BlockMap.try_emplace(NewBlock, Cycle);
-      Cycle = Cycle->getParentCycle();
-    }
+  CycleT *Cycle = getCycleContaining(Pred, Succ);
+  while (Cycle) {
+    // FixMe: Appending NewBlock is fine as a set of blocks in a cycle. When
+    // printing cycle NewBlock is at the end of list but it should be in the
+    // middle to represent actual traversal of a cycle.
+    Cycle->appendBlock(NewBlock);
+    BlockMap.try_emplace(NewBlock, Cycle);
+    Cycle = Cycle->getParentCycle();
   }
   assert(validateTree());
 }
@@ -392,6 +390,36 @@ auto GenericCycleInfo<ContextT>::getCycle(const BlockT *Block) const
   return BlockMap.lookup(Block);
 }
 
+/// \brief Find the innermost cycle containing both given blocks.
+///
+/// \returns the innermost cycle containing both \p A and \p B or nullptr if
+///          they are not both contained in any cycle.
+template <typename ContextT>
+auto GenericCycleInfo<ContextT>::getCycleContaining(const BlockT *A,
+                                                    const BlockT *B) const
+    -> CycleT * {
+  CycleT *CycleA = getCycle(A);
+  CycleT *CycleB = getCycle(B);
+  if (!CycleA || !CycleB)
+    return nullptr;
+
+  // If CycleA and CycleB have different depth replace them with parents until
+  // they have the same depth.
+  while (CycleA->getDepth() > CycleB->getDepth())
+    CycleA = CycleA->getParentCycle();
+  while (CycleB->getDepth() > CycleA->getDepth())
+    CycleB = CycleB->getParentCycle();
+
+  // CycleA and CycleB are at same depth but may be disjoint, replace them with
+  // parents until we find cycle that contains both or we run out of parents.
+  while (CycleA != CycleB) {
+    CycleA = CycleA->getParentCycle();
+    CycleB = CycleB->getParentCycle();
+  }
+
+  return CycleA;
+}
+
 /// \brief get the depth for the cycle which containing a given block.
 ///
 /// \returns the depth for the innermost cycle containing \p Block or 0 if it is
diff --git a/llvm/include/llvm/ADT/GenericCycleInfo.h b/llvm/include/llvm/ADT/GenericCycleInfo.h
index 18be7eb4a6cca9d..b6be59b75a6c1ab 100644
--- a/llvm/include/llvm/ADT/GenericCycleInfo.h
+++ b/llvm/include/llvm/ADT/GenericCycleInfo.h
@@ -261,6 +261,7 @@ template <typename ContextT> class GenericCycleInfo {
   const ContextT &getSSAContext() const { return Context; }
 
   CycleT *getCycle(const BlockT *Block) const;
+  CycleT *getCycleContaining(const BlockT *A, const BlockT *B) const;
   unsigned getCycleDepth(const BlockT *Block) const;
   CycleT *getTopLevelParentCycle(BlockT *Block);
 

[nhaehnle]

LGTM algorithmically, but I have an interface design nit inline.

[nhaehnle]

Please instead make this a member CycleT *getSmallestCommonCycle(CycleT *A, CycleT *B) const so that it is more generally useful.

[petar-avramovic]

Done

[nhaehnle]

The idea was that the method would take Cycle arguments...

[ssahasra]

This should take cycles as arguments. It is more likely that the client has already located the cycles for A and B, so we can avoid the calls to getCycle() inside this function.

[ssahasra]

This can't work, right? The BlockMap should point to the direct parent of NewBlock, but this will keep updating it up the tree.

[ssahasra]

With all these details to handle, I think we should have a separate function like CycleInfo::addBlockToCycle() or something.

[petar-avramovic]

OK, so it should add block to cycle and all of the parent cycles and also to innermost and topLevel BlockMap. Are there more details that I am not aware of?