From 0e33beac6f8a777cae08a1d46a2c11d352061067 Mon Sep 17 00:00:00 2001 From: tnowicki Date: Mon, 30 Dec 2024 16:41:54 -0500 Subject: [PATCH 1/6] [Coroutines] Unit tests for addSplitRefRecursiveFunctions. --- llvm/unittests/Analysis/LazyCallGraphTest.cpp | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/llvm/unittests/Analysis/LazyCallGraphTest.cpp b/llvm/unittests/Analysis/LazyCallGraphTest.cpp index 6ca233a50f31f..311c5d470f2cc 100644 --- a/llvm/unittests/Analysis/LazyCallGraphTest.cpp +++ b/llvm/unittests/Analysis/LazyCallGraphTest.cpp @@ -3027,4 +3027,58 @@ TEST(LazyCallGraphTest, AddSplitFunctions5) { EXPECT_EQ(RC, CG.lookupRefSCC(F2N)); EXPECT_EQ(CG.postorder_ref_scc_end(), I); } + +TEST(LazyCallGraphTest, AddSplitFunctions6) { + LLVMContext Context; + std::unique_ptr M = parseAssembly(Context, "define void @f() {\n" + " ret void\n" + "}\n"); + LazyCallGraph CG = buildCG(*M); + + Function &F = lookupFunction(*M, "f"); + LazyCallGraph::Node &FN = CG.get(F); + + // Force the graph to be fully expanded. + CG.buildRefSCCs(); + auto I = CG.postorder_ref_scc_begin(); + LazyCallGraph::RefSCC *ORC = &*I++; + EXPECT_EQ(CG.postorder_ref_scc_end(), I); + + auto *G1 = Function::Create(F.getFunctionType(), F.getLinkage(), + F.getAddressSpace(), "g1", F.getParent()); + auto *G2 = Function::Create(F.getFunctionType(), F.getLinkage(), + F.getAddressSpace(), "g2", F.getParent()); + BasicBlock *G1BB = BasicBlock::Create(Context, "", G1); + BasicBlock *G2BB = BasicBlock::Create(Context, "", G2); + // Create g1 -ref-> g2 and g2 has no references. + (void)CastInst::CreatePointerCast(G2, PointerType::getUnqual(Context), "", + G1BB); + (void)ReturnInst::Create(Context, G1BB); + (void)ReturnInst::Create(Context, G2BB); + + // Create f -ref-> g1 and f -ref-> g2. + (void)CastInst::CreatePointerCast(G1, PointerType::getUnqual(Context), "", + F.getEntryBlock().begin()); + (void)CastInst::CreatePointerCast(G2, PointerType::getUnqual(Context), "", + F.getEntryBlock().begin()); + + EXPECT_FALSE(verifyModule(*M, &errs())); + + CG.addSplitRefRecursiveFunctions(F, SmallVector({G1, G2})); + + LazyCallGraph::Node *G1N = CG.lookup(*G1); + EXPECT_TRUE(G1N); + LazyCallGraph::Node *G2N = CG.lookup(*G2); + EXPECT_TRUE(G2N); + + I = CG.postorder_ref_scc_begin(); + LazyCallGraph::RefSCC *RC1 = &*I++; + EXPECT_EQ(2, RC1->size()); + EXPECT_EQ(RC1, CG.lookupRefSCC(*G1N)); + EXPECT_EQ(RC1, CG.lookupRefSCC(*G2N)); + LazyCallGraph::RefSCC *RC2 = &*I++; + EXPECT_EQ(RC2, ORC); + EXPECT_EQ(RC2, CG.lookupRefSCC(FN)); + EXPECT_EQ(CG.postorder_ref_scc_end(), I); +} } From d66bb3993b5477d03f142667e135722f888d6423 Mon Sep 17 00:00:00 2001 From: tnowicki Date: Thu, 14 Nov 2024 16:02:39 -0500 Subject: [PATCH 2/6] [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 5aa36bfc36d46..724bd0ff416c8 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 e36064283b6c18b1b0d4f38059fe97360a6305a5 Mon Sep 17 00:00:00 2001 From: tnowicki Date: Mon, 25 Nov 2024 11:40:35 -0500 Subject: [PATCH 3/6] [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 289e9c3990bcc..4bc0021a194f3 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 NewFunctions); diff --git a/llvm/lib/Analysis/LazyCallGraph.cpp b/llvm/lib/Analysis/LazyCallGraph.cpp index 724bd0ff416c8..7771f9c4fe37a 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")); } From 9331a6135752428d4f5531f18555582a90f97674 Mon Sep 17 00:00:00 2001 From: tnowicki Date: Fri, 10 Jan 2025 13:05:50 -0500 Subject: [PATCH 4/6] [Coroutines] Updated with reviewer feedback --- llvm/include/llvm/Analysis/LazyCallGraph.h | 13 ++++++++----- llvm/lib/Analysis/LazyCallGraph.cpp | 19 +------------------ 2 files changed, 9 insertions(+), 23 deletions(-) diff --git a/llvm/include/llvm/Analysis/LazyCallGraph.h b/llvm/include/llvm/Analysis/LazyCallGraph.h index 4bc0021a194f3..9abd73948f5df 100644 --- a/llvm/include/llvm/Analysis/LazyCallGraph.h +++ b/llvm/include/llvm/Analysis/LazyCallGraph.h @@ -1081,12 +1081,15 @@ class LazyCallGraph { /// Add new ref-recursive functions split/outlined from an existing 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 new functions may only reference other functions that the original + /// function did. The new functions may reference the original function. New + /// functions must not call other new functions or the original function. /// - /// Mark the original function as referencing all new functions. - /// Mark all new functions as referencing each other. + /// Marks the original function as referencing all new functions. + /// + /// The CG must be updated following the use of this helper, for example with + /// updateCGAndAnalysisManagerForCGSCCPass(), to ensure the RefSCCs and SCCs + /// are correctly identified. void addSplitRefRecursiveFunctions(Function &OriginalFunction, ArrayRef NewFunctions); diff --git a/llvm/lib/Analysis/LazyCallGraph.cpp b/llvm/lib/Analysis/LazyCallGraph.cpp index 7771f9c4fe37a..eb91b7673afee 100644 --- a/llvm/lib/Analysis/LazyCallGraph.cpp +++ b/llvm/lib/Analysis/LazyCallGraph.cpp @@ -1733,23 +1733,6 @@ 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 @@ -1793,7 +1776,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")); } From 62882ec6f9a0647b1a7f5f3ddd25cd1e9d51cb99 Mon Sep 17 00:00:00 2001 From: tnowicki Date: Wed, 12 Feb 2025 23:33:52 -0500 Subject: [PATCH 5/6] [Coroutines] Updated with reviewer feedback --- llvm/include/llvm/Analysis/LazyCallGraph.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/llvm/include/llvm/Analysis/LazyCallGraph.h b/llvm/include/llvm/Analysis/LazyCallGraph.h index 9abd73948f5df..3d98b781b8dc0 100644 --- a/llvm/include/llvm/Analysis/LazyCallGraph.h +++ b/llvm/include/llvm/Analysis/LazyCallGraph.h @@ -1087,9 +1087,11 @@ class LazyCallGraph { /// /// Marks the original function as referencing all new functions. /// - /// The CG must be updated following the use of this helper, for example with - /// updateCGAndAnalysisManagerForCGSCCPass(), to ensure the RefSCCs and SCCs - /// are correctly identified. + /// It is not necessary for each new function to reference all other new + /// functions. Spurious/missing ref edges are allowed. The new functions + /// are considered to be a RefSCC. If any new function references the + /// original function they are all considered to be part of the original + /// functions RefSCC. void addSplitRefRecursiveFunctions(Function &OriginalFunction, ArrayRef NewFunctions); From 92c936285f86ea30160fa5e1fc2ca96a35379bd0 Mon Sep 17 00:00:00 2001 From: tnowicki Date: Wed, 12 Feb 2025 23:34:46 -0500 Subject: [PATCH 6/6] [Coroutines] Updated with reviewer feedback --- llvm/include/llvm/Analysis/LazyCallGraph.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/include/llvm/Analysis/LazyCallGraph.h b/llvm/include/llvm/Analysis/LazyCallGraph.h index 3d98b781b8dc0..a69ed91b0465e 100644 --- a/llvm/include/llvm/Analysis/LazyCallGraph.h +++ b/llvm/include/llvm/Analysis/LazyCallGraph.h @@ -1091,7 +1091,7 @@ class LazyCallGraph { /// functions. Spurious/missing ref edges are allowed. The new functions /// are considered to be a RefSCC. If any new function references the /// original function they are all considered to be part of the original - /// functions RefSCC. + /// function's RefSCC. void addSplitRefRecursiveFunctions(Function &OriginalFunction, ArrayRef NewFunctions);