-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[LTO][Pipelines][Coro] Handle coroutines in LTO pipeline #126168
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.
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
[LTO][Pipelines][Coro] Handle coroutines in LTO pipeline #126168
Conversation
Created using spr 1.3.4
@llvm/pr-subscribers-lto Author: Vitaly Buka (vitalybuka) ChangesThinLTO delays handling of coroutines to ThinLTO backend. In this case we have left-over coroutines which crash in codegen. Issue #104525. Full diff: https://github.com/llvm/llvm-project/pull/126168.diff 4 Files Affected:
diff --git a/llvm/lib/Passes/PassBuilderPipelines.cpp b/llvm/lib/Passes/PassBuilderPipelines.cpp
index c6cf6cdbe9390f8..e631e03c8384080 100644
--- a/llvm/lib/Passes/PassBuilderPipelines.cpp
+++ b/llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -14,6 +14,7 @@
///
//===----------------------------------------------------------------------===//
+#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/Statistic.h"
#include "llvm/Analysis/AliasAnalysis.h"
#include "llvm/Analysis/BasicAliasAnalysis.h"
@@ -1816,6 +1817,17 @@ PassBuilder::buildLTODefaultPipeline(OptimizationLevel Level,
// in the current module.
MPM.addPass(CrossDSOCFIPass());
+ MPM.addPass(CoroEarlyPass());
+
+ auto Exit = llvm::make_scope_exit([&]() {
+ MPM.addPass(CoroCleanupPass());
+
+ invokeFullLinkTimeOptimizationLastEPCallbacks(MPM, Level);
+
+ // Emit annotation remarks.
+ addAnnotationRemarksPass(MPM);
+ });
+
if (Level == OptimizationLevel::O0) {
// The WPD and LowerTypeTest passes need to run at -O0 to lower type
// metadata and intrinsics.
@@ -1826,11 +1838,6 @@ PassBuilder::buildLTODefaultPipeline(OptimizationLevel Level,
MPM.addPass(LowerTypeTestsPass(nullptr, nullptr,
lowertypetests::DropTestKind::Assume));
- invokeFullLinkTimeOptimizationLastEPCallbacks(MPM, Level);
-
- // Emit annotation remarks.
- addAnnotationRemarksPass(MPM);
-
return MPM;
}
@@ -1910,11 +1917,6 @@ PassBuilder::buildLTODefaultPipeline(OptimizationLevel Level,
MPM.addPass(LowerTypeTestsPass(nullptr, nullptr,
lowertypetests::DropTestKind::Assume));
- invokeFullLinkTimeOptimizationLastEPCallbacks(MPM, Level);
-
- // Emit annotation remarks.
- addAnnotationRemarksPass(MPM);
-
return MPM;
}
@@ -1983,7 +1985,11 @@ PassBuilder::buildLTODefaultPipeline(OptimizationLevel Level,
// If we didn't decide to inline a function, check to see if we can
// transform it to pass arguments by value instead of by reference.
- MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(ArgumentPromotionPass()));
+ CGSCCPassManager CGPM;
+ CGPM.addPass(ArgumentPromotionPass());
+ CGPM.addPass(CoroSplitPass(Level != OptimizationLevel::O0));
+ CGPM.addPass(CoroAnnotationElidePass());
+ MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM)));
FunctionPassManager FPM;
// The IPO Passes may leave cruft around. Clean up after them.
@@ -2135,11 +2141,6 @@ PassBuilder::buildLTODefaultPipeline(OptimizationLevel Level,
if (PTO.CallGraphProfile)
MPM.addPass(CGProfilePass(/*InLTOPostLink=*/true));
- invokeFullLinkTimeOptimizationLastEPCallbacks(MPM, Level);
-
- // Emit annotation remarks.
- addAnnotationRemarksPass(MPM);
-
return MPM;
}
diff --git a/llvm/test/Other/new-pm-O0-defaults.ll b/llvm/test/Other/new-pm-O0-defaults.ll
index e8131ac7fab45ac..db450269414e3ef 100644
--- a/llvm/test/Other/new-pm-O0-defaults.ll
+++ b/llvm/test/Other/new-pm-O0-defaults.ll
@@ -47,12 +47,14 @@
; CHECK-THINLTO-NEXT: Running pass: EliminateAvailableExternallyPass
; CHECK-THINLTO-NEXT: Running pass: GlobalDCEPass
; CHECK-LTO: Running pass: CrossDSOCFIPass on [module]
+; CHECK-LTO-NEXT: CoroEarlyPass
; CHECK-LTO-NEXT: Running pass: WholeProgramDevirtPass
; CHECK-LTO-NEXT: Running analysis: InnerAnalysisManagerProxy
; CHECK-LTO-NEXT: Running pass: LowerTypeTestsPass
; CHECK-LTO-NEXT: Running pass: LowerTypeTestsPass
; CHECK-CORO-NEXT: Running pass: AnnotationRemarksPass
; CHECK-CORO-NEXT: Running analysis: TargetLibraryAnalysis
+; CHECK-LTO-NEXT: CoroCleanupPass
; CHECK-LTO-NEXT: Running pass: AnnotationRemarksPass
; CHECK-LTO-NEXT: Running analysis: TargetLibraryAnalysis
; CHECK-NEXT: Running pass: PrintModulePass
diff --git a/llvm/test/Other/new-pm-lto-defaults.ll b/llvm/test/Other/new-pm-lto-defaults.ll
index 86480c5115748d5..d6e88bd7b5c50a5 100644
--- a/llvm/test/Other/new-pm-lto-defaults.ll
+++ b/llvm/test/Other/new-pm-lto-defaults.ll
@@ -35,6 +35,7 @@
; CHECK-EP: Running pass: NoOpModulePass
; CHECK-O: Running pass: CrossDSOCFIPass
+; CHECK-O-NEXT: CoroEarlyPass
; CHECK-O-NEXT: Running pass: OpenMPOptPass
; CHECK-O-NEXT: Running pass: GlobalDCEPass
; CHECK-O-NEXT: Running pass: InferFunctionAttrsPass
@@ -86,6 +87,8 @@
; CHECK-O23SZ-NEXT: Running pass: OpenMPOptPass
; CHECK-O23SZ-NEXT: Running pass: GlobalDCEPass
; CHECK-O23SZ-NEXT: Running pass: ArgumentPromotionPass
+; CHECK-O23SZ-NEXT: CoroSplitPass on (foo)
+; CHECK-O23SZ-NEXT: CoroAnnotationElidePass on (foo)
; CHECK-O23SZ-NEXT: Running pass: InstCombinePass
; CHECK-EP-PEEPHOLE-NEXT: Running pass: NoOpFunctionPass
; CHECK-O23SZ-NEXT: Running pass: ConstraintEliminationPass
@@ -156,6 +159,7 @@
; CHECK-O23SZ-NEXT: Running pass: GlobalDCEPass
; CHECK-O23SZ-NEXT: Running pass: RelLookupTableConverterPass
; CHECK-O23SZ-NEXT: Running pass: CGProfilePass
+; CHECK-O-NEXT: CoroCleanupPass
; CHECK-EP-NEXT: Running pass: NoOpModulePass
; CHECK-O-NEXT: Running pass: AnnotationRemarksPass on foo
; CHECK-O-NEXT: Running pass: PrintModulePass
diff --git a/llvm/test/ThinLTO/X86/coro.ll b/llvm/test/ThinLTO/X86/coro.ll
new file mode 100644
index 000000000000000..9094062b2fe275f
--- /dev/null
+++ b/llvm/test/ThinLTO/X86/coro.ll
@@ -0,0 +1,21 @@
+; RUN: llvm-as %s -o %t1.bc
+; RUN: llvm-lto2 run %t1.bc -o %t2.o -r=%t1.bc,test,plx -r=%t1.bc,extern_func,plx -save-temps
+; RUN: llvm-dis %t2.o.0.5.precodegen.bc -o - | FileCheck %s --implicit-check-not=llvm.coro
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-fuchsia"
+
+declare void @extern_func()
+
+; CHECK: define {{.*}} void @test(
+define void @test(ptr %hdl) {
+ call void @llvm.coro.resume(ptr %hdl)
+ call void @llvm.coro.destroy(ptr %hdl)
+ call i1 @llvm.coro.done(ptr %hdl)
+ ret void
+}
+
+declare void @llvm.coro.resume(ptr)
+declare void @llvm.coro.destroy(ptr)
+declare i1 @llvm.coro.done(ptr)
+
|
Created using spr 1.3.4
Created using spr 1.3.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with the caveat that I don't know the correct relative ordering of the Coro passes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should be consistent between Full and ThinLTO and also skip coroutine passes in prelink FullLTO so we don't run coroutine passes twice with FullLTO
Good idea. Still to avoid regressions, I propose to land this patch as-is. It's essentially NFC for all who don't see the crash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this patch is a regression in the sense that now we run coroutine passes twice in FullLTO, but I think the coroutine passes are all idempotent so that should be ok, so I think landing this separately should be fine
Created using spr 1.3.4 [skip ci]
Helper for #126168. `Phase` will be used in followup patches.
@vogelsgesang I'd like to merge it if no other feedback? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/12542 Here is the relevant piece of the build log for the reference
|
Helper for llvm#126168. `Phase` will be used in followup patches.
ThinLTO delays handling of coroutines to ThinLTO backend. However it's usually possible to use ThinLTO prelink objects for FullLTO. In this case we have left-over coroutines which crash in codegen. Issue llvm#104525.
Helper for llvm#126168. `Phase` will be used in followup patches.
ThinLTO delays handling of coroutines to ThinLTO backend. However it's usually possible to use ThinLTO prelink objects for FullLTO. In this case we have left-over coroutines which crash in codegen. Issue llvm#104525.
Helper for llvm#126168. `Phase` will be used in followup patches.
ThinLTO delays handling of coroutines to ThinLTO backend. However it's usually possible to use ThinLTO prelink objects for FullLTO. In this case we have left-over coroutines which crash in codegen. Issue llvm#104525.
``` if (!isLTOPostLink(Phase)) CoroPM.addPass(CoroEarlyPass()); if (!isLTOPreLink(Phase)) // Other Coro passes ``` Followup to #126168.
ThinLTO delays handling of coroutines to ThinLTO backend.
However it's usually possible to use ThinLTO prelink object for FullLTO.
In this case we have left-over coroutines which crash in codegen.
Issue #104525.