Skip to content

[Coroutines] properly update CallGraph in CoroSplit #107935

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 3 commits into from
Sep 12, 2024
Merged

[Coroutines] properly update CallGraph in CoroSplit #107935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
00da984
[Coroutines] Make CoroSplit properly update CallGraph
yuxuanchen1997 Sep 9, 2024
a622067
use reviewer suggested style
yuxuanchen1997 Sep 12, 2024
61755c8
Revert "use reviewer suggested style"
yuxuanchen1997 Sep 12, 2024
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
18 changes: 12 additions & 6 deletions llvm/lib/Transforms/Coroutines/CoroSplit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2080,12 +2080,13 @@ splitCoroutine(Function &F, SmallVectorImpl<Function *> &Clones,
return Shape;
}

static void updateCallGraphAfterCoroutineSplit(
static LazyCallGraph::SCC &updateCallGraphAfterCoroutineSplit(
LazyCallGraph::Node &N, const coro::Shape &Shape,
const SmallVectorImpl<Function *> &Clones, LazyCallGraph::SCC &C,
LazyCallGraph &CG, CGSCCAnalysisManager &AM, CGSCCUpdateResult &UR,
FunctionAnalysisManager &FAM) {
Comment on lines +2083 to 2087
Copy link
Member

Choose a reason for hiding this comment

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

nit: the signature is slightly odd to me. It looks better to make it the below CurrentSCC to be an output parameter of the function. So that we can avoid the odd the pattern

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I can't. It's not just to keep it consistent with the semantics of updateCGAndAnalysisManagerForCGSCCPass. The LazyCallGraph::SCC &C is an lvalue ref and cannot be rebound. If I do C = update... it will invoke the Move Assignment operator for LazyCallGraph::SCC. Not sure the consequence will be intended.

My other option is to make this input a LazyCallGraph::SCC ** or LazyCallGraph::SCC *&, which is also not very obvious.

Copy link
Member

Choose a reason for hiding this comment

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

Can't we make the type of C to LazyCallGraph::SCC *&?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I noted that the signature is actually very similar to LLVM's updateCGAndAnalysisManagerForPass() signature. Perhaps it is better to be consistent with that? https://llvm.org/doxygen/CGSCCPassManager_8cpp.html#a490117b63072462d035a6933fdb94c1f

Copy link
Member Author

Choose a reason for hiding this comment

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

I noted that the signature is actually very similar to LLVM's updateCGAndAnalysisManagerForPass() signature. Perhaps it is better to be consistent with that? https://llvm.org/doxygen/CGSCCPassManager_8cpp.html#a490117b63072462d035a6933fdb94c1f

Perhaps. Or perhaps the answer is to rewrite updateCGAndAnalysisManagerForPass. I really don't like the current semantics. I will see what I can do in another patch.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ChuanqiXu9 I am reverting this suggestion for now. We can do a refactor to change all updateCGAndAnalysisManagerFor(Function|CGSCC)?Pass usage once we decide on a new design.

Copy link
Member Author

Choose a reason for hiding this comment

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

$ rg "updateCGAndAnalysisManagerFor(Function|CGSCC)?Pass"
llvm/unittests/Analysis/CGSCCPassManagerTest.cpp
1212:            updateCGAndAnalysisManagerForFunctionPass(CG, C, H2N, AM, UR, FAM);
1258:            updateCGAndAnalysisManagerForFunctionPass(CG, C, H2N, AM, UR, FAM);
1370:            updateCGAndAnalysisManagerForCGSCCPass(CG, C, H2N, AM, UR, FAM));
1407:        updateCGAndAnalysisManagerForFunctionPass(CG, C, H2N, AM, UR, FAM),
1438:            updateCGAndAnalysisManagerForCGSCCPass(CG, C, FN, AM, UR, FAM));
1469:        updateCGAndAnalysisManagerForFunctionPass(CG, C, FN, AM, UR, FAM),
1508:            updateCGAndAnalysisManagerForCGSCCPass(CG, C, FN, AM, UR, FAM));
1545:    ASSERT_DEATH(updateCGAndAnalysisManagerForCGSCCPass(CG, C, FN, AM, UR, FAM),
1777:              updateCGAndAnalysisManagerForCGSCCPass(CG, C, *N, AM, UR, FAM))
1867:          updateCGAndAnalysisManagerForCGSCCPass(CG, C, *N, AM, UR, FAM))
1923:        // Check that updateCGAndAnalysisManagerForCGSCCPass() after
1925:        updateCGAndAnalysisManagerForCGSCCPass(CG, *CG.lookupSCC(*N1), *N1, AM,
1987:          updateCGAndAnalysisManagerForCGSCCPass(CG, C, *N, AM, UR, FAM))

llvm/lib/Transforms/Utils/CallGraphUpdater.cpp
68:    updateCGAndAnalysisManagerForCGSCCPass(*LCG, *C, N, *AM, *UR, *FAM);

llvm/lib/Transforms/IPO/Inliner.cpp
497:    C = &updateCGAndAnalysisManagerForCGSCCPass(CG, *C, N, AM, UR, FAM);

llvm/lib/Transforms/Coroutines/CoroSplit.cpp
2108:    updateCGAndAnalysisManagerForCGSCCPass(CG, C, N, AM, UR, FAM);
2114:  updateCGAndAnalysisManagerForFunctionPass(CG, C, N, AM, UR, FAM);

llvm/lib/Analysis/CGSCCPassManager.cpp
561:      CurrentC = &updateCGAndAnalysisManagerForFunctionPass(CG, *CurrentC, *N,
876:static LazyCallGraph::SCC &updateCGAndAnalysisManagerForPass(
925:        // promoted before updateCGAndAnalysisManagerForPass runs.
1178:LazyCallGraph::SCC &llvm::updateCGAndAnalysisManagerForFunctionPass(
1182:  return updateCGAndAnalysisManagerForPass(G, InitialC, N, AM, UR, FAM,
1185:LazyCallGraph::SCC &llvm::updateCGAndAnalysisManagerForCGSCCPass(
1189:  return updateCGAndAnalysisManagerForPass(G, InitialC, N, AM, UR, FAM,

llvm/include/llvm/Analysis/CGSCCPassManager.h
419:LazyCallGraph::SCC &updateCGAndAnalysisManagerForFunctionPass(
430:LazyCallGraph::SCC &updateCGAndAnalysisManagerForCGSCCPass(

This is manageable refactoring I'd say. Let's do this in a separate patch.


auto *CurrentSCC = &C;
if (!Clones.empty()) {
switch (Shape.ABI) {
case coro::ABI::Switch:
Expand All @@ -2105,13 +2106,16 @@ static void updateCallGraphAfterCoroutineSplit(
}

// Let the CGSCC infra handle the changes to the original function.
updateCGAndAnalysisManagerForCGSCCPass(CG, C, N, AM, UR, FAM);
CurrentSCC = &updateCGAndAnalysisManagerForCGSCCPass(CG, *CurrentSCC, N, AM,
UR, FAM);
}

// Do some cleanup and let the CGSCC infra see if we've cleaned up any edges
// to the split functions.
postSplitCleanup(N.getFunction());
updateCGAndAnalysisManagerForFunctionPass(CG, C, N, AM, UR, FAM);
CurrentSCC = &updateCGAndAnalysisManagerForFunctionPass(CG, *CurrentSCC, N,
AM, UR, FAM);
return *CurrentSCC;
}

/// Replace a call to llvm.coro.prepare.retcon.
Expand Down Expand Up @@ -2200,6 +2204,7 @@ PreservedAnalyses CoroSplitPass::run(LazyCallGraph::SCC &C,
if (Coroutines.empty() && PrepareFns.empty())
return PreservedAnalyses::all();

auto *CurrentSCC = &C;
// Split all the coroutines.
for (LazyCallGraph::Node *N : Coroutines) {
Function &F = N->getFunction();
Expand All @@ -2211,7 +2216,8 @@ PreservedAnalyses CoroSplitPass::run(LazyCallGraph::SCC &C,
coro::Shape Shape =
splitCoroutine(F, Clones, FAM.getResult<TargetIRAnalysis>(F),
OptimizeFrame, MaterializableCallback);
updateCallGraphAfterCoroutineSplit(*N, Shape, Clones, C, CG, AM, UR, FAM);
CurrentSCC = &updateCallGraphAfterCoroutineSplit(
*N, Shape, Clones, *CurrentSCC, CG, AM, UR, FAM);

auto &ORE = FAM.getResult<OptimizationRemarkEmitterAnalysis>(F);
ORE.emit([&]() {
Expand All @@ -2223,14 +2229,14 @@ PreservedAnalyses CoroSplitPass::run(LazyCallGraph::SCC &C,

if (!Shape.CoroSuspends.empty()) {
// Run the CGSCC pipeline on the original and newly split functions.
UR.CWorklist.insert(&C);
UR.CWorklist.insert(CurrentSCC);
for (Function *Clone : Clones)
UR.CWorklist.insert(CG.lookupSCC(CG.get(*Clone)));
}
}

for (auto *PrepareFn : PrepareFns) {
replaceAllPrepares(PrepareFn, CG, C);
replaceAllPrepares(PrepareFn, CG, *CurrentSCC);
}

return PreservedAnalyses::none();
Expand Down
38 changes: 38 additions & 0 deletions llvm/test/Transforms/Coroutines/gh107139-split-in-scc.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
; Verify that we don't crash on mutually recursive coroutines
; RUN: opt < %s -passes='cgscc(coro-split)' -S | FileCheck %s

target triple = "x86_64-redhat-linux-gnu"

; CHECK-LABEL: define void @foo
define void @foo() presplitcoroutine personality ptr null {
entry:

%0 = call token @llvm.coro.id(i32 0, ptr null, ptr null, ptr null)
%1 = call ptr @llvm.coro.begin(token %0, ptr null)
%2 = call token @llvm.coro.save(ptr null)
%3 = call i8 @llvm.coro.suspend(token none, i1 false)
%4 = call token @llvm.coro.save(ptr null)
; CHECK: call void @bar(ptr null, ptr null)
call void @llvm.coro.await.suspend.void(ptr null, ptr null, ptr @bar)
ret void
}

; CHECK-LABEL: define void @bar({{.*}})
define void @bar(ptr %0, ptr %1) {
entry:
; CHECK: call void @foo()
call void @foo()
ret void
}

; CHECK-LABEL: @foo.resume({{.*}})
; CHECK-LABEL: @foo.destroy({{.*}})
; CHECK-LABEL: @foo.cleanup({{.*}})

declare token @llvm.coro.id(i32, ptr readnone, ptr nocapture readonly, ptr) #0
declare ptr @llvm.coro.begin(token, ptr writeonly) nounwind
declare token @llvm.coro.save(ptr) nomerge nounwind
declare void @llvm.coro.await.suspend.void(ptr, ptr, ptr)
declare i8 @llvm.coro.suspend(token, i1) nounwind

attributes #0 = { nocallback nofree nosync nounwind willreturn memory(argmem: read) }
Loading