Skip to content

[Coroutines][LazyCallGraph] addSplitRefRecursiveFunctions allows spurious ref edges between new functions. #116285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Feb 14, 2025
Merged

[Coroutines][LazyCallGraph] addSplitRefRecursiveFunctions allows spurious ref edges between new functions. #116285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0e33bea
[Coroutines] Unit tests for addSplitRefRecursiveFunctions.
TylerNowicki Dec 30, 2024
d66bb39
[Coroutines][LazyCallGraph] resumes are not really SCC
Nov 14, 2024
e360642
[Coroutines] [LazyCallGraph] Make all new functions reference eachother.
TylerNowicki Nov 25, 2024
9331a61
[Coroutines] Updated with reviewer feedback
TylerNowicki Jan 10, 2025
62882ec
[Coroutines] Updated with reviewer feedback
TylerNowicki Feb 13, 2025
92c9362
[Coroutines] Updated with reviewer feedback
TylerNowicki Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions llvm/include/llvm/Analysis/LazyCallGraph.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1082,11 +1082,16 @@ 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.
/// function did. The new functions may reference the original function. New
/// functions must not call other new functions or the original function.
///
/// The original function must reference (not call) all new functions.
/// All new functions must reference (not call) each other.
/// Marks the original function as referencing all new functions.
///
/// 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
/// function's RefSCC.
void addSplitRefRecursiveFunctions(Function &OriginalFunction,
ArrayRef<Function *> NewFunctions);

Expand Down
6 changes: 4 additions & 2 deletions llvm/lib/Analysis/LazyCallGraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1720,6 +1720,7 @@ void LazyCallGraph::addSplitRefRecursiveFunctions(
for (Function *NewFunction : NewFunctions) {
Node &NewN = initNode(*NewFunction);

// Make the original function reference each new function
Comment on lines 1721 to +1723
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't initNode above populate the edges out of the node? that should already be adding the edges in the code you're adding below

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this doesn't add edges between nodes if there are no edges between them. Right now when we create a continuation we clone the whole thing, the entire function and fixup the entry point, then it is added to the LazyCallGraph (so there is a phi in the continuation with references to all other new continuations), then do a clean-up that removes all the unused code including the phi.

Obviously cloning the whole function, just for the phi references, then removing all of the unused code is wasted compile-time. I will be making a change that prunes the BBs while creating the continuation. The only problem is now the phi with references to the other continuations is not there. But that phi being there is a bit artificial anyway since we remove it. Instead, this function can add the edges that were not represented in the continuation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LazyCallGraph models the actual IR, so if there aren't any references in the actual IR, there shouldn't be an edge in LazyCallGraph. The LazyCallGraph APIs should be called after all transformations are done. I think I'm not fully understanding your comment, a concrete example would be helpful.

Copy link
Collaborator Author

@TylerNowicki TylerNowicki Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you describe differs from what is happening right now (without my patch). Here are some more details on how it works right now. First for the coroutine function we create a single/unified return block for each suspend point and create a phi in the return BB that takes a reference to a continuation function. We create one empty continuation function for each suspend point. Then for each continuation function we clone the contents of the coroutine function in its entirety and fix-up the entry point. Notice that the return BB and its phi are retained in each continuation function. Then we call addSplitRefRecursiveFunctions and then we do simplify-cfg. The simplify-cfg removes all of the unnecessary blocks and as a result it will remove some or all of the edges in the phi we added to the return BB.

For example, if we have a coroutine like

foo() {
 // part A
 suspend1:
 // part B
 suspend2:
 // part C
 exit:
}

Then we first generate

foo() {
 // part A
 suspend1:
 // part B
 suspend2:
 // part C
 exit:v
 returnBB:
 phi = [continuationB suspend1, continuationC, suspend2]
}

continuationB () {
 // part A
 suspend1:
 // part B  <-- entry point
 suspend2:
 // part C
 exit:
 returnBB:
 phi = [continuationB suspend1, continuationC, suspend2]
}

continuationC () {
 // part A
 suspend1:
 // part B
 suspend2:
 // part C <-- entry point
 exit:
 returnBB:
 phi = [continuationB suspend1, continuationC, suspend2]
}

At this point we call addSplitRefRecursiveFunctions, adding the new continuation functions to the call graph and they will form a (Ref)SCC.

However, then we do simplify-cfg and the resulting functions look like

foo() {
 // part A
 returnBB:
 phi = [continuationB suspend1]
}

continuationB () {
 // part B  <-- entry point
 returnBB
 phi = [continuationC, suspend2]
}

continuationC () {
 // part C <-- entry point
 exit:
}

So, the question is, what is the correct call-graph? Based on previous comments I assumed we actually wanted to keep the current behavior and keep the continuation functions in the same (Ref)SCC. See an earlier comment by John.

adding a ref from every funclet to every non-initial funclet is definitely the conservatively correct way to handle all of the lowerings that use non-unified continuations (which is everything but the switch lowering).

The Async method and the returned-continuation method both use addSplitRefRecursiveFunctions to add their new continuation functions to the call graph. As John said we don't need to worry about the switch method because it uses addSplitFunction to add its funclets to the call graph.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a unit test that resembles the above example.

Copy link
Collaborator Author

@TylerNowicki TylerNowicki Jan 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just have addSplitRefRecursiveFunctions() call updateCGAndAnalysisManagerForCGSCCPass() then? Otherwise, it feels like we are just reinventing the function to identify the SCCs and RefSCCs in addSplitRefRecursiveFunctions(). The alternative is before adding the new functions to the CallGraph I add unreachable blocks to create 'fake' references between the new functions on the incoming edges of an unused phi node to give the new functions the appearance of being a RefSCC. Then after addSplitRefRecursiveFunctions() I delete those and call updateCGAndAnalysisManagerForCGSCCPass().

Both of these options don't sound great. It would be a lot better if the rule was that the callgraph matches the IR following completion of the pass. It seems overly restrictive to require the call-graph to match the IR at arbitrary points in the pass transformation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm confused based on the description and the previous discussion. Are you planning on first creating all the split functions in full (referencing each other), then one by one simplify-cfg each one, or are you planning on simplifying the cfg as you clone, meaning there's never actually a point where we have a fully connected RefSCC? The discussion above seems to imply the former, and the PR description seems to imply the latter.

If it's the former, then something like

coroSplit(Function& F, LazyCallGraph& LCG) {
  std::vector<Function *> SplitFunctions = doSplit(F);
  LCG.addSplitRefRecursiveFunctions(F, SplitFunctions);
  for (Function& NewF: SplitFunctions) {
    runSimplifyCfg(NewF);
    updateCGAndAnalysisManagerForFunctionPass(LCG, NewF);
    // updateCGAndAnalysisManagerForFunctionPass handles all possible transformations done by simplify-cfg
    // updateCGAndAnalysisManagerForCGSCCPass is not necessary since we only did function pass transformations since the last LCG update
  }
}

is simple and should work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My request is the latter option you gave. Sorry for the confusion. I will create an RFC to explain my request more concisely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another clarifying question, does the original function still always reference all newly split functions?

Copy link
Collaborator Author

@TylerNowicki TylerNowicki Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

another clarifying question, does the original function still always reference all newly split functions?

Yes, that is the case. I don't see that changing.

OriginalN->insertEdgeInternal(NewN, Edge::Kind::Ref);

// Check if there is any edge from any new function back to any function in
Expand Down Expand Up @@ -1775,8 +1776,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) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it seems like we're not actually adding the spurious ref edges since Node::populateSlow() doesn't actually add the ref edge since it doesn't exist in the IR, so the RefSCC being created isn't a valid strongly connected component. we need to manually add the fake ref edges (which feels weird but oh well) as opposed to relaxing this check to allow the edge to not exist

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion on this. I will make another PR for this change soon.

(!N1->lookup(N2)->isCall() &&
"Edges between new functions must be ref edges"));
}
}
#endif
Expand Down
54 changes: 54 additions & 0 deletions llvm/unittests/Analysis/LazyCallGraphTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Module> 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<Function *, 1>({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);
}
}