-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
[fatlto] Add coroutine passes when using FatLTO with ThinLTO #134434
Conversation
@llvm/pr-subscribers-clang @llvm/pr-subscribers-coroutines Author: Paul Kirth (ilovepi) ChangesWhen coroutines are used w/ both -ffat-lto-objects and -flto=thin, Fixes #134409. Full diff: https://github.com/llvm/llvm-project/pull/134434.diff 2 Files Affected:
diff --git a/clang/test/CodeGenCoroutines/pr134409.cpp b/clang/test/CodeGenCoroutines/pr134409.cpp
new file mode 100644
index 0000000000000..3f3d95e191594
--- /dev/null
+++ b/clang/test/CodeGenCoroutines/pr134409.cpp
@@ -0,0 +1,42 @@
+// An end-to-end test to make sure coroutine passes are added for thinlto.
+
+// RUN: %clang_cc1 -std=c++23 -ffat-lto-objects -flto=thin -emit-llvm %s -O3 -o - \
+// RUN: | FileCheck %s
+
+#include "Inputs/coroutine.h"
+
+class BasicCoroutine {
+public:
+ struct Promise {
+ BasicCoroutine get_return_object() { return BasicCoroutine {}; }
+
+ void unhandled_exception() noexcept { }
+
+ void return_void() noexcept { }
+
+ std::suspend_never initial_suspend() noexcept { return {}; }
+ std::suspend_never final_suspend() noexcept { return {}; }
+ };
+ using promise_type = Promise;
+};
+
+// COM: match the embedded module, so we don't match something in it by accident.
+// CHECK: @llvm.embedded.object = {{.*}}
+// CHECK: @llvm.compiler.used = {{.*}}
+
+BasicCoroutine coro() {
+// CHECK: define {{.*}} void @_Z4corov() {{.*}} {
+// CHECK-NEXT: entry:
+// CHECK-NEXT: ret void
+// CHECK-NEXT: }
+ co_return;
+}
+
+int main() {
+// CHECK: define {{.*}} i32 @main() {{.*}} {
+// CHECK-NEXT: entry:
+// CHECK-NEXT: ret i32 0
+// CHECK-NEXT: }
+ coro();
+}
+
diff --git a/llvm/lib/Passes/PassBuilderPipelines.cpp b/llvm/lib/Passes/PassBuilderPipelines.cpp
index a18b36ba40754..4b15e0fb5c2a7 100644
--- a/llvm/lib/Passes/PassBuilderPipelines.cpp
+++ b/llvm/lib/Passes/PassBuilderPipelines.cpp
@@ -1688,10 +1688,15 @@ PassBuilder::buildFatLTODefaultPipeline(OptimizationLevel Level, bool ThinLTO,
MPM.addPass(
LowerTypeTestsPass(nullptr, nullptr, lowertypetests::DropTestKind::All));
- // Use the ThinLTO post-link pipeline with sample profiling
- if (ThinLTO && PGOOpt && PGOOpt->Action == PGOOptions::SampleUse)
+ // ModuleSimplification does not run the coroutine passes for ThinLTOPreLink,
+ // so we need the coroutine passes to run for ThinLTO builds, otherwise they
+ // will miscompile.
+ if (ThinLTO) {
+ // TODO: determine how to only run the ThinLTODefaultPipeline when using
+ // sample profiling. Ideally, we'd be able to still use the module
+ // optimization pipeline, with additional cleanups for coroutines.
MPM.addPass(buildThinLTODefaultPipeline(Level, /*ImportSummary=*/nullptr));
- else {
+ } else {
// otherwise, just use module optimization
MPM.addPass(
buildModuleOptimizationPipeline(Level, ThinOrFullLTOPhase::None));
|
cc: @mcatanzaro |
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.
Switching to the ThinLTO post-link pipeline will have a big impact on optimization behavior and compile-time. I think it would be safer to make a change along the lines of #126168, i.e. to schedule the necessary passes in the FatLTO pipeline. Without having looked into it closely, it would probably be okay to just schedule them before the buildModuleOptimizationPipeline call.
hmm, I tried adding some of the individual passes, and ran into some other errors. Let me give that another try w/ that PR as an example. |
6c3ea0b
to
a491924
Compare
// TODO: replace w/ buildCoroWrapper() when it takes phase and level into | ||
// consideration. | ||
CGSCCPassManager CGPM; | ||
CGPM.addPass(CoroSplitPass(Level != OptimizationLevel::O0)); | ||
CGPM.addPass(CoroAnnotationElidePass()); | ||
MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM))); | ||
MPM.addPass(CoroCleanupPass()); |
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.
Also possible to do MPM.addPass(buildModuleSimplificationPipeline(Level, ThinOrFullLTOPhase::ThinLTOPostLink));
That seems to generate better code here, since it allows the tail call in the test to get removed. But that seems a bit expensive, just to give the inliner a second chance.
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.
I think the current PR is a good way to start, as it avoids the miscompilation in a minimal way that is suitable for backporting.
Using the module simplification pipeline here (+ the module optimization pipeline) would effectively be the same as using the ThinLTO post-link pipeline.
I ran some tests, and it looks like adding -ffat-lto-objects
adds about 10% overhead to the clang build (https://llvm-compile-time-tracker.com/compare.php?from=44923d8631fb28b4de54d4210762f256c3894cef&to=2bdf721c2d37af6fcbd931d963f586478cb55f17&stat=instructions:u). On top of that, switching this from using the module optimization pipeline to the ThinLTO post-link pipeline (your original PR) adds an extra 4%: https://llvm-compile-time-tracker.com/compare.php?from=2bdf721c2d37af6fcbd931d963f586478cb55f17&to=63666418fbe19f30bf971796747a751b4e1c57f3&stat=instructions:u
Adding 4% overhead is not great, but also not terrible, so maybe it's worth it to avoid pipeline compatibility issues in a principled way. It does make the codegen for FatLTO ELF diverge more from a normal compilation though.
Ideally we'd be able to rerun the simplification pipeline, but skip the inliner pipeline for already optimized functions. (I think @aeubanks had a prototype that did that for the actual ThinLTO scenario, by looking at available_externally functions. The situation here is somewhat different.)
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.
Hmm, 10% seems a bit high for overhead on build times, though we haven't used it too much w/ ThinLTO in our toolchain, so maybe that's it?
Looking at our build times when we enabled it in our toolchain, we saw about a 2.5% slowdown in total build time, but a 22% improvement in test time (ninja check-*). Overall that ended up being about 4.4% speedup in total time.
So, I'm not surprised it slowed down for just the build, but I am surprised it added a full 10%. Well, I guess I/O can have a lot of variance between machines, so maybe that's enough to explain it, since for ThinLTO it probably more than doubles the size of the .o
.
When coroutines are used w/ both -ffat-lto-objects and -flto=thin, the coroutine passes are not added to the optimization pipelines. Ensure they are added before ModuleOptimization to generate a working ELF object. Fixes #134409.
a491924
to
80c3615
Compare
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, but PR title needs adjustment.
/cherry-pick 268c065 |
/pull-request #134711 |
…4434) When coroutines are used w/ both -ffat-lto-objects and -flto=thin, the coroutine passes are not added to the optimization pipelines. Ensure they are added before ModuleOptimization to generate a working ELF object. Fixes llvm#134409. (cherry picked from commit 268c065)
When coroutines are used w/ both -ffat-lto-objects and -flto=thin,
the coroutine passes are not added to the optimization pipelines.
Ensure they are added before ModuleOptimization to generate a
working ELF object.
Fixes #134409.