[Pipelines] Do not run CoroSplit and CoroCleanup in LTO pre-link pipeline #90310
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-coroutines @llvm/pr-subscribers-clang Author: Wei Wang (apolloww) ChangesSkip CoroSplit and CoroCleanup in pre-lto pipeline so that CoroElide can happen after callee coroutine is imported into caller's module in ThinLTO. Full diff: https://github.com/llvm/llvm-project/pull/90310.diff 6 Files Affected:
diff --git a/clang/test/CodeGenCoroutines/coro-elide-thinlto.cpp b/clang/test/CodeGenCoroutines/coro-elide-thinlto.cpp
new file mode 100644
index 00000000000000..293aef6781677f
--- /dev/null
+++ b/clang/test/CodeGenCoroutines/coro-elide-thinlto.cpp
@@ -0,0 +1,77 @@
+// This tests that the coroutine elide optimization could happen succesfully with ThinLTO.
+// This test is adapted from coro-elide.cpp and splits functions into two files.
+//
+// RUN: split-file %s %t
+// RUN: %clang --target=x86_64-linux -std=c++20 -O2 -flto=thin -I %S -c %t/coro-elide-callee.cpp -o coro-elide-callee.o
+// RUN: %clang --target=x86_64-linux -std=c++20 -O2 -flto=thin -I %S -c %t/coro-elide-caller.cpp -o coro-elide-caller.o
+// RUN: llvm-lto -thinlto coro-elide-callee.o coro-elide-caller.o -o summary
+// RUN: %clang_cc1 -O2 -x ir coro-elide-caller.o -fthinlto-index=summary.thinlto.bc -emit-llvm -o - | FileCheck %s
+
+//--- coro-elide-task.h
+#pragma once
+#include "Inputs/coroutine.h"
+
+struct Task {
+ struct promise_type {
+ struct FinalAwaiter {
+ bool await_ready() const noexcept { return false; }
+ template <typename PromiseType>
+ std::coroutine_handle<> await_suspend(std::coroutine_handle<PromiseType> h) noexcept {
+ if (!h)
+ return std::noop_coroutine();
+ return h.promise().continuation;
+ }
+ void await_resume() noexcept {}
+ };
+ Task get_return_object() noexcept {
+ return std::coroutine_handle<promise_type>::from_promise(*this);
+ }
+ std::suspend_always initial_suspend() noexcept { return {}; }
+ FinalAwaiter final_suspend() noexcept { return {}; }
+ void unhandled_exception() noexcept {}
+ void return_value(int x) noexcept {
+ _value = x;
+ }
+ std::coroutine_handle<> continuation;
+ int _value;
+ };
+
+ Task(std::coroutine_handle<promise_type> handle) : handle(handle) {}
+ ~Task() {
+ if (handle)
+ handle.destroy();
+ }
+
+ struct Awaiter {
+ bool await_ready() const noexcept { return false; }
+ void await_suspend(std::coroutine_handle<void> continuation) noexcept {}
+ int await_resume() noexcept {
+ return 43;
+ }
+ };
+
+ auto operator co_await() {
+ return Awaiter{};
+ }
+
+private:
+ std::coroutine_handle<promise_type> handle;
+};
+
+//--- coro-elide-callee.cpp
+#include "coro-elide-task.h"
+Task task0() {
+ co_return 43;
+}
+
+//--- coro-elide-caller.cpp
+#include "coro-elide-task.h"
+
+Task task0();
+
+Task task1() {
+ co_return co_await task0();
+}
+
+// CHECK-LABEL: define{{.*}} void @_Z5task1v.resume
+// CHECK-NOT: {{.*}}_Znwm
diff --git a/llvm/lib/Passes/PassBuilderPipelines.cpp b/llvm/lib/Passes/PassBuilderPipelines.cpp
index 90ba3b541553e2..4ca8aff479b0ff 100644
--- a/llvm/lib/Passes/PassBuilderPipelines.cpp
+++ b/llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -963,7 +963,8 @@ PassBuilder::buildInlinerPipeline(OptimizationLevel Level,
MainCGPipeline.addPass(createCGSCCToFunctionPassAdaptor(
RequireAnalysisPass<ShouldNotRunFunctionPassesAnalysis, Function>()));
- MainCGPipeline.addPass(CoroSplitPass(Level != OptimizationLevel::O0));
+ if (!isLTOPreLink(Phase))
+ MainCGPipeline.addPass(CoroSplitPass(Level != OptimizationLevel::O0));
// Make sure we don't affect potential future NoRerun CGSCC adaptors.
MIWP.addLateModulePass(createModuleToFunctionPassAdaptor(
@@ -1005,8 +1006,10 @@ PassBuilder::buildModuleInlinerPipeline(OptimizationLevel Level,
buildFunctionSimplificationPipeline(Level, Phase),
PTO.EagerlyInvalidateAnalyses));
- MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(
- CoroSplitPass(Level != OptimizationLevel::O0)));
+
+ if (!isLTOPreLink(Phase))
+ MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(
+ CoroSplitPass(Level != OptimizationLevel::O0)));
return MPM;
}
@@ -1183,7 +1186,8 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
// and argument promotion.
MPM.addPass(DeadArgumentEliminationPass());
- MPM.addPass(CoroCleanupPass());
+ if (!isLTOPreLink(Phase))
+ MPM.addPass(CoroCleanupPass());
// Optimize globals now that functions are fully simplified.
MPM.addPass(GlobalOptPass());
diff --git a/llvm/test/Other/new-pm-defaults.ll b/llvm/test/Other/new-pm-defaults.ll
index 51fb93daa4dfa6..ebfed7b687e2ce 100644
--- a/llvm/test/Other/new-pm-defaults.ll
+++ b/llvm/test/Other/new-pm-defaults.ll
@@ -224,12 +224,14 @@
; CHECK-O-NEXT: Running pass: PostOrderFunctionAttrsPass
; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}ShouldNotRunFunctionPassesAnalysis
; CHECK-O-NEXT: Running analysis: ShouldNotRunFunctionPassesAnalysis
-; CHECK-O-NEXT: Running pass: CoroSplitPass
+; CHECK-DEFAULT-NEXT: Running pass: CoroSplitPass
+; CHECK-LTO-NOT: Running pass: CoroSplitPass
; CHECK-O-NEXT: Running pass: InvalidateAnalysisPass<{{.*}}ShouldNotRunFunctionPassesAnalysis
; CHECK-O-NEXT: Invalidating analysis: ShouldNotRunFunctionPassesAnalysis
; CHECK-O-NEXT: Invalidating analysis: InlineAdvisorAnalysis
; CHECK-O-NEXT: Running pass: DeadArgumentEliminationPass
-; CHECK-O-NEXT: Running pass: CoroCleanupPass
+; CHECK-DEFAULT-NEXT: Running pass: CoroCleanupPass
+; CHECK-LTO-NOT: Running pass: CoroCleanupPass
; CHECK-O-NEXT: Running pass: GlobalOptPass
; CHECK-O-NEXT: Running pass: GlobalDCEPass
; CHECK-DEFAULT-NEXT: Running pass: EliminateAvailableExternallyPass
diff --git a/llvm/test/Other/new-pm-thinlto-prelink-defaults.ll b/llvm/test/Other/new-pm-thinlto-prelink-defaults.ll
index 6486639e07b49c..e2fd74306f80c3 100644
--- a/llvm/test/Other/new-pm-thinlto-prelink-defaults.ll
+++ b/llvm/test/Other/new-pm-thinlto-prelink-defaults.ll
@@ -183,12 +183,10 @@
; CHECK-O-NEXT: Running pass: PostOrderFunctionAttrsPass
; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}ShouldNotRunFunctionPassesAnalysis
; CHECK-O-NEXT: Running analysis: ShouldNotRunFunctionPassesAnalysis
-; CHECK-O-NEXT: Running pass: CoroSplitPass
; CHECK-O-NEXT: Running pass: InvalidateAnalysisPass<{{.*}}ShouldNotRunFunctionPassesAnalysis
; CHECK-O-NEXT: Invalidating analysis: ShouldNotRunFunctionPassesAnalysis
; CHECK-O-NEXT: Invalidating analysis: InlineAdvisorAnalysis
; CHECK-O-NEXT: Running pass: DeadArgumentEliminationPass
-; CHECK-O-NEXT: Running pass: CoroCleanupPass
; CHECK-O-NEXT: Running pass: GlobalOptPass
; CHECK-O-NEXT: Running pass: GlobalDCEPass
; CHECK-EXT: Running pass: {{.*}}::Bye
diff --git a/llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll b/llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
index 09f9f0f48baddb..13a63bbe4d9cad 100644
--- a/llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
+++ b/llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll
@@ -182,12 +182,10 @@
; CHECK-O-NEXT: Running pass: PostOrderFunctionAttrsPass
; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}ShouldNotRunFunctionPassesAnalysis
; CHECK-O-NEXT: Running analysis: ShouldNotRunFunctionPassesAnalysis
-; CHECK-O-NEXT: Running pass: CoroSplitPass
; CHECK-O-NEXT: Running pass: InvalidateAnalysisPass<{{.*}}ShouldNotRunFunctionPassesAnalysis
; CHECK-O-NEXT: Invalidating analysis: ShouldNotRunFunctionPassesAnalysis
; CHECK-O-NEXT: Invalidating analysis: InlineAdvisorAnalysis
; CHECK-O-NEXT: Running pass: DeadArgumentEliminationPass
-; CHECK-O-NEXT: Running pass: CoroCleanupPass
; CHECK-O-NEXT: Running pass: GlobalOptPass
; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis on bar
; CHECK-O-NEXT: Running pass: GlobalDCEPass
diff --git a/llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll b/llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
index 47bdbfd2d357d4..3130da86fa9908 100644
--- a/llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
+++ b/llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll
@@ -147,12 +147,10 @@
; CHECK-O-NEXT: Running pass: PostOrderFunctionAttrsPass
; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}ShouldNotRunFunctionPassesAnalysis
; CHECK-O-NEXT: Running analysis: ShouldNotRunFunctionPassesAnalysis
-; CHECK-O-NEXT: Running pass: CoroSplitPass
; CHECK-O-NEXT: Running pass: InvalidateAnalysisPass<{{.*}}ShouldNotRunFunctionPassesAnalysis
; CHECK-O-NEXT: Invalidating analysis: ShouldNotRunFunctionPassesAnalysis
; CHECK-O-NEXT: Invalidating analysis: InlineAdvisorAnalysis
; CHECK-O-NEXT: Running pass: DeadArgumentEliminationPass
-; CHECK-O-NEXT: Running pass: CoroCleanupPass
; CHECK-O-NEXT: Running pass: GlobalOptPass
; CHECK-O-NEXT: Running pass: GlobalDCEPass
; CHECK-O-NEXT: Running pass: AnnotationRemarksPass on foo
|
apolloww
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
This change seems to causing the auto-roller for emscripten to fail due to a crash in LTO. See https://ci.chromium.org/ui/p/emscripten-releases/builders/luci.emscripten-releases.ci/linux-test-suites |
|
The function being compiled in the crash looks like this one: https://github.com/emscripten-core/emscripten/blob/56547157a37e25fb31f9193461a2c88bdaa44385/system/lib/embind/bind.cpp#L66-L68 |
|
hmm, this looks like |
|
OK, so Mono LTO backend pipeline does not have any coro passes. We can add coroutine passes to |
| // | ||
| // RUN: split-file %s %t | ||
| // RUN: %clang --target=x86_64-linux -std=c++20 -O2 -flto=thin -I %S -c %t/coro-elide-callee.cpp -o coro-elide-callee.o | ||
| // RUN: %clang --target=x86_64-linux -std=c++20 -O2 -flto=thin -I %S -c %t/coro-elide-caller.cpp -o coro-elide-caller.o |
There was a problem hiding this comment.
This test case requires an X86 backend. Those of us who have embraced the AArch64-only future are encountering test errors when running this test.
(maybe it's just me)
There was a problem hiding this comment.
Just landed #90672 to limit the test to x86 linux targets. Did that fix the test failure?
There was a problem hiding this comment.
Yep! Thank you for the quick fix.
…ink pipeline (#90310)" and related patches This change is incorrect when thinlto and asan are enabled, and this can be observed by adding `-fsanitize=address` to the provided coro-elide-thinlto.cpp test. It results in the error "Coroutines cannot handle non static allocas yet", and ASan introduces a dynamic alloca. In other words, we must preserve the invariant that CoroSplit runs before ASan. If we move CoroSplit to the post post-link compile stage, ASan has to be moved to the post-link compile stage first. It would also be correct to make CoroSplit handle dynamic allocas so the pass ordering doesn't matter, but sanitizer instrumentation really ought to be last, after coroutine splitting. This reverts commit bafc5f4. This reverts commit b1b1bfa. This reverts commit 0232b77. This reverts commit fb2d305. This reverts commit 1cb3371. This reverts commit cd68d7b.
|
Hey, I went ahead and reverted this patch in aa0776d because it is incompatible with ASan ThinLTO builds. I think you have uncovered a latent issue in the ASan + thinlto configuration that is not really the fault of this change, but I didn't see a path to fixing forward in the short term, so I reverted this to get this configuration working again. I think the next step is to follow up with @vitalybuka to figure out how to reorder the ASan instrumentation pass in the thinlto pipeline. |
Oh, that's unfortunate. Could you explain more about the issue? |
There are other benefits to moving Asan to post-link ThinLTO stage, but it's likely will not trivial. Last time I tried, it caused other miscompiles which needed investigation. I still hope to get back to that sometime later, but there is no particular timeline. If this patch is important to land soon I see the following options:
Depending on the nature of the issues, this may apply to other sanitizers. |
|
To be clear, you can repro the issue with the test case you have, just add |
|
Got some time to revisit this. I tried the repro, and think there are couple issues here. function import can't handle non-function aliaseeAfter adding In fact, this has nothing to do with coroutines. Asan would create alias for other GVs as well and trigger the same crash. For example, the following test case (mostly copied from ; RUN: split-file %s %t
; RUN: opt -passes=asan,name-anon-globals -module-summary %t/main.ll -o main.bc
; RUN: opt -passes=asan,name-anon-globals -module-summary %t/alias.ll -o alias.bc
; RUN: llvm-lto --thinlto main.bc local_alias.bc -o summary
; RUN: %clang -O2 -x ir main.bc -fthinlto-index=summary.thinlto.bc
;--- main.ll
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
declare i32 @foo(i32)
define i32 @main() local_unnamed_addr {
%1 = call i32 @foo(i32 1)
ret i32 %1
}
;--- alias.ll
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
@a = dso_local global [2 x i32] zeroinitializer, align 4
@b = private global [2 x i32] zeroinitializer, align 4
@c = internal global [2 x i32] zeroinitializer, align 4
@_ZL1d = unnamed_addr global [2 x i32] zeroinitializer, align 4
; Function Attrs: nounwind sanitize_address uwtable
define i32 @foo(i32 %M) {
entry:
%M.addr = alloca i32, align 4
store i32 %M, ptr %M.addr, align 4
store volatile i32 6, ptr getelementptr inbounds ([2 x i32], ptr @a, i64 2, i64 0), align 4
%0 = load i32, ptr %M.addr, align 4
%idxprom = sext i32 %0 to i64
%arrayidx = getelementptr inbounds [2 x i32], ptr @a, i64 0, i64 %idxprom
%1 = load volatile i32, ptr %arrayidx, align 4
ret i32 %1
}In real use case though, I doubt we'd see this exact crash because the alias wouldn't be imported as it has private linkage (not 100% sure). why it shows up here is because after Either way, I think this issue can be resolve by handling non-function aliasee in function import.
|
Skip CoroSplit and CoroCleanup in LTO pre-link pipeline so that CoroElide can happen after callee coroutine is imported into caller's module in ThinLTO.