[llvm] [Coroutines][LazyCallGraph] resumes are not really SCC (PR #116285)

[Tyler Nowicki via llvm-commits]

https://github.com/TylerNowicki updated https://github.com/llvm/llvm-project/pull/116285

>From f28812f77003e32962527f9091d6770516d4c269 Mon Sep 17 00:00:00 2001
From: tnowicki <tnowicki.nowicki at amd.com>
Date: Thu, 14 Nov 2024 16:02:39 -0500
Subject: [PATCH 1/2] [Coroutines][LazyCallGraph] resumes are not really SCC

In this debug code the check assumes the lookup find an object. If it
does not it tries to dereference the nullptr. This patch adds a check,
however, notice it does not require the object to exist.

This method (addSplitRefRecursiveFunctions) was added for coroutines
with continuations. The split coroutine transform generates the
continuations by cloning the entire function then changing the entry
block and a few other things. This results in a lot of dead code that is
kept around only to satisfy a few assumptions then is removed.

With the deadcode the original function and its continuations appear to
be SCC. However, when the deadcode is removed they are not SCC. Consider
a simple coroutine { begin ... suspend ... suspend ... end }. After
deadcode is eliminated the ramp references resume0, resume0 references
resume1, etc...

To be efficient it is necessary to split coroutines without generating
deadcode and this removese the unnecessary references (on a phi's edges).
This does not satisfy one of the conditions of addSplitRefRecursiveFunctions:
"All new functions must reference (not call) each other.".

>From my testing so far it seems that this condition is not necessary and
it is only this debug code that checks for it.

Can we safely remove the "All new functions must reference (not call) each
other." requirement of addSplitRefRecursiveFunctions?

If not, what alternative do we have so we avoid cloning deadcode? (In
our use case the deadcode can be a particular problem due to its size).
---
 llvm/lib/Analysis/LazyCallGraph.cpp | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/llvm/lib/Analysis/LazyCallGraph.cpp b/llvm/lib/Analysis/LazyCallGraph.cpp
index 5aa36bfc36d468..724bd0ff416c8a 100644
--- a/llvm/lib/Analysis/LazyCallGraph.cpp
+++ b/llvm/lib/Analysis/LazyCallGraph.cpp
@@ -1775,8 +1775,9 @@ void LazyCallGraph::addSplitRefRecursiveFunctions(
       if (F1 == F2)
         continue;
       Node &N2 = get(*F2);
-      assert(!N1->lookup(N2)->isCall() &&
-             "Edges between new functions must be ref edges");
+      assert(!N1->lookup(N2) ||
+             (!N1->lookup(N2)->isCall() &&
+              "Edges between new functions must be ref edges"));
     }
   }
 #endif

>From 84a0c804d5746fc6d8120cce1f055ba6e4253b68 Mon Sep 17 00:00:00 2001
From: tnowicki <tyler.nowicki at amd.com>
Date: Mon, 25 Nov 2024 11:40:35 -0500
Subject: [PATCH 2/2] [Coroutines] [LazyCallGraph] Make all new functions
 reference eachother.

---
 llvm/include/llvm/Analysis/LazyCallGraph.h | 10 +++++-----
 llvm/lib/Analysis/LazyCallGraph.cpp        | 20 +++++++++++++++++++-
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/llvm/include/llvm/Analysis/LazyCallGraph.h b/llvm/include/llvm/Analysis/LazyCallGraph.h
index 289e9c3990bcc6..4bc0021a194f34 100644
--- a/llvm/include/llvm/Analysis/LazyCallGraph.h
+++ b/llvm/include/llvm/Analysis/LazyCallGraph.h
@@ -1081,12 +1081,12 @@ class LazyCallGraph {
 
   /// Add new ref-recursive functions split/outlined from an existing function.
   ///
-  /// The new functions may only reference other functions that the original
-  /// function did. The new functions may reference (not call) the original
-  /// function.
+  /// The new functions may only reference the original function or other
+  /// functions that the original function did. New functions must not call
+  /// other new functions.
   ///
-  /// The original function must reference (not call) all new functions.
-  /// All new functions must reference (not call) each other.
+  /// Mark the original function as referencing all new functions.
+  /// Mark all new functions as referencing each other.
   void addSplitRefRecursiveFunctions(Function &OriginalFunction,
                                      ArrayRef<Function *> NewFunctions);
 
diff --git a/llvm/lib/Analysis/LazyCallGraph.cpp b/llvm/lib/Analysis/LazyCallGraph.cpp
index 724bd0ff416c8a..7771f9c4fe37ae 100644
--- a/llvm/lib/Analysis/LazyCallGraph.cpp
+++ b/llvm/lib/Analysis/LazyCallGraph.cpp
@@ -1720,6 +1720,7 @@ void LazyCallGraph::addSplitRefRecursiveFunctions(
   for (Function *NewFunction : NewFunctions) {
     Node &NewN = initNode(*NewFunction);
 
+    // Make the original function reference each new function
     OriginalN->insertEdgeInternal(NewN, Edge::Kind::Ref);
 
     // Check if there is any edge from any new function back to any function in
@@ -1732,6 +1733,23 @@ void LazyCallGraph::addSplitRefRecursiveFunctions(
     }
   }
 
+  for (Function *NewFunction : NewFunctions) {
+    Node &NewN = get(*NewFunction);
+    for (Function *OtherNewFunction : NewFunctions) {
+      if (NewFunction == OtherNewFunction)
+        continue;
+
+      Node &OtherNewN = get(*OtherNewFunction);
+
+      // Don't add an edge if one already exists.
+      if (NewN->lookup(OtherNewN))
+        continue;
+
+      // Make the new function reference each other new function
+      NewN->insertEdgeInternal(OtherNewN, Edge::Kind::Ref);
+    }
+  }
+
   RefSCC *NewRC;
   if (ExistsRefToOriginalRefSCC) {
     // If there is any edge from any new function to any function in the
@@ -1775,7 +1793,7 @@ void LazyCallGraph::addSplitRefRecursiveFunctions(
       if (F1 == F2)
         continue;
       Node &N2 = get(*F2);
-      assert(!N1->lookup(N2) ||
+      assert(N1->lookup(N2) &&
              (!N1->lookup(N2)->isCall() &&
               "Edges between new functions must be ref edges"));
     }