[Coroutines][6/6] Clang schedules new passes

[modocache]

Summary

Depends on https://reviews.llvm.org/D71902.

The last in a series of six patches that ports the LLVM coroutines
passes to the new pass manager infrastructure.

This patch has Clang schedule the new coroutines passes when the
-fexperimental-new-pass-manager option is used. With this and the
previous 5 patches, Clang is capable of building and successfully
running the test suite of large coroutines projects such as
https://github.com/lewissbaker/cppcoro with
ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER=On.

[junparser]

clang/lib/CodeGen/BackendUtil.cpp

1273

Since coro elision depends on other optimization pass(inline and so on) implicitly, how can we adjust the pipeline to achieve this.

[modocache]

clang/lib/CodeGen/BackendUtil.cpp

1273

One option would be to use the new pass manager's registration callbacks, like PassBuilder::registerPipelineStartEPCallback or PassBuilder::registerOptimizerLastEPCallback. These work similarly to the old pass manager's PassManagerBuilder::addExtension. That's something that I think would be good to improve in a follow-up patch, but let me know if you'd rather see it in this one.

[junparser]

There is another issue we should consider: clang is crashed when compile coroutine with -disable-llvm-passes and output an object file.
Is it reasonable to run coroutine passes even -disable-llvm-passes is enabled?

clang/lib/CodeGen/BackendUtil.cpp

1273

yes, please. It should be done in this patch sets.

[wenlei]

clang/lib/CodeGen/BackendUtil.cpp

1274

Manually scheduling the 2nd coro-split passes in the same CGSCC pipeline would make the resume/suspend funclet ineligible for the bulk of CSGSS opts, given the split point is relatively early. The implication would be discouraging the use of coroutine in performance critical path. I would love to see this being addressed before we claim coroutine is ready for new PM.

As commented in the 2nd patch, we may not need the devirt trick used with legacy PM, instead for new PM, we could try to leverage CGSCCUpdateResult without involving artificial indirect call and devirt (or follow how outlining is handled by new PM).

[modocache]

clang/lib/CodeGen/BackendUtil.cpp

1273

Will do!

1274

I would love to see this being addressed before we claim coroutine is ready for new PM.

I apologize, since I didn't intend to make such a claim. In http://lists.llvm.org/pipermail/llvm-dev/2019-December/137835.html I explained that these 6 patches were focused on lowering coroutine intrinsics. My goal was to resolve https://bugs.llvm.org/show_bug.cgi?id=42867, so that C++ coroutines code didn't trigger a fatal error in ISel.

That being said, I'm happy to make changes here. I'll send updates for D71899 and this patch.

[modocache]

I'm currently working on ensuring that CGSCC optimizations are rerun to optimize coroutine funclets -- the primary feedback I received on this and on D71899 -- but I just realized I didn't respond to one comment on this set of reviews, from @junparser:

There is another issue we should consider: clang is crashed when compile coroutine with -disable-llvm-passes and output an object file.

It's always been the case, since the coroutine intrinsics and passes were first added to LLVM, that attempting to codegen without first running coroutine passes would cause a crash during instruction selection. So clang -Xclang -disable-llvm-passes -c has always crashed Clang during LLVM ISel, as it does in this example that uses Clang 9.0.0 and the legacy pass manager: https://godbolt.org/z/Mj2R5G

Personally I'm of the opinion that this is less than ideal... I may be wrong, but I don't think there are very many other C++ features that *require* Clang to run LLVM passes (perhaps the always_inline attribute requires LLVM passes to be run for correctness? I'm not sure). So I would like to see this eventually addressed somehow.

Is it reasonable to run coroutine passes even -disable-llvm-passes is enabled?

My personal opinion is that this would not be reasonable. The option -disable-llvm-passes should, from my point of view, prevent any and all LLVM passes from being run. I also frequently make use of this option when debugging the LLVM IR being output for C++ coroutines code, so if -disable-llvm-passes didn't disable coroutines passes, I'd need another option that did (-disable-llvm-passes-no-really-even-coroutine-passes-them-too 😅).

All this being said, considering this behavior has existed in the legacy PM since day one, I think we should start a separate discussion on if/how to change that behavior. I'm working on an update for these patches to address funclet optimization, but the update will not change the fact that coroutine passes are not run when -disable-llvm-passes is specified. I think that's an orthogonal issue.

[junparser]

In D71903#1804016, @modocache wrote:

I'm currently working on ensuring that CGSCC optimizations are rerun to optimize coroutine funclets -- the primary feedback I received on this and on D71899 -- but I just realized I didn't respond to one comment on this set of reviews, from @junparser:

There is another issue we should consider: clang is crashed when compile coroutine with -disable-llvm-passes and output an object file.

It's always been the case, since the coroutine intrinsics and passes were first added to LLVM, that attempting to codegen without first running coroutine passes would cause a crash during instruction selection. So clang -Xclang -disable-llvm-passes -c has always crashed Clang during LLVM ISel, as it does in this example that uses Clang 9.0.0 and the legacy pass manager: https://godbolt.org/z/Mj2R5G

Personally I'm of the opinion that this is less than ideal... I may be wrong, but I don't think there are very many other C++ features that *require* Clang to run LLVM passes (perhaps the always_inline attribute requires LLVM passes to be run for correctness? I'm not sure). So I would like to see this eventually addressed somehow.

Is it reasonable to run coroutine passes even -disable-llvm-passes is enabled?

My personal opinion is that this would not be reasonable. The option -disable-llvm-passes should, from my point of view, prevent any and all LLVM passes from being run. I also frequently make use of this option when debugging the LLVM IR being output for C++ coroutines code, so if -disable-llvm-passes didn't disable coroutines passes, I'd need another option that did (-disable-llvm-passes-no-really-even-coroutine-passes-them-too 😅).

All this being said, considering this behavior has existed in the legacy PM since day one, I think we should start a separate discussion on if/how to change that behavior. I'm working on an update for these patches to address funclet optimization, but the update will not change the fact that coroutine passes are not run when -disable-llvm-passes is specified. I think that's an orthogonal issue.

make sense to me, thanks!

[modocache]

Thanks for the reviews. Based on my latest revision of D71899, the coro-split diff, this patch now no longer manually enqueues coro-split twice. In addition, it uses the PassBuilder registration callbacks to enqueue coroutine passes in appropriate spots in the pipeline. coro-split now restarts the entire MainCGPipeline that's built as part of module optimization. As discussed, the coroutine passes are enqueued at -O0, and they're not enqueued when -disable-llvm-passes is specified.

[modocache]

Update tests -- we now re-run the SCC pass, but don't insert the coroutine funclets into the SCC, so we no longer see the funclets in the output being tested here.

[modocache]

Initialize PipelineTuningOptions properly.

[modocache]

Rebase past D72547.

[modocache]

Clean up coroutine intrinsics as part of the ThinLTO pre-link pipeline.

[modocache]

Rebase on top of the latest version of D71902.

clang/lib/CodeGen/BackendUtil.cpp

 #include "llvm/Support/PrettyStackTrace.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/Timer.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetOptions.h"
 #include "llvm/Transforms/Coroutines.h"
+#include "llvm/Transforms/Coroutines/CoroCleanup.h"
+#include "llvm/Transforms/Coroutines/CoroEarly.h"
+#include "llvm/Transforms/Coroutines/CoroElide.h"
+#include "llvm/Transforms/Coroutines/CoroSplit.h"
 #include "llvm/Transforms/IPO.h"
 #include "llvm/Transforms/IPO/AlwaysInliner.h"
 #include "llvm/Transforms/IPO/LowerTypeTests.h"
 #include "llvm/Transforms/IPO/PassManagerBuilder.h"
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
 #include "llvm/Transforms/InstCombine/InstCombine.h"
 #include "llvm/Transforms/Instrumentation.h"
 #include "llvm/Transforms/Instrumentation/AddressSanitizer.h"
case 2:
       return PassBuilder::OptimizationLevel::Oz;
     }
 
   case 3:
     return PassBuilder::OptimizationLevel::O3;
   }
 }
 
+static void addCoroutinePassesAtO0(ModulePassManager &MPM,
+                                   const LangOptions &LangOpts,
+                                   const CodeGenOptions &CodeGenOpts) {
+  if (!LangOpts.Coroutines)
+    return;
+
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroEarlyPass()));
+
+  CGSCCPassManager CGPM(CodeGenOpts.DebugPassManager);
+  CGPM.addPass(CoroSplitPass());
+  CGPM.addPass(createCGSCCToFunctionPassAdaptor(CoroElidePass()));
+  MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM)));
+
+  MPM.addPass(createModuleToFunctionPassAdaptor(CoroCleanupPass()));
+}
+
 static void addSanitizersAtO0(ModulePassManager &MPM,
                               const Triple &TargetTriple,
                               const LangOptions &LangOpts,
                               const CodeGenOptions &CodeGenOpts) {
   auto ASanPass = [&](SanitizerMask Mask, bool CompileKernel) {
     MPM.addPass(RequireAnalysisPass<ASanGlobalsMetadataAnalysis, Module>());
     bool Recover = CodeGenOpts.SanitizeRecover.has(Mask);
     MPM.addPass(createModuleToFunctionPassAdaptor(AddressSanitizerPass(
void EmitAssemblyHelper::EmitAssemblyWithNewPassManager(
 
   PipelineTuningOptions PTO;
   PTO.LoopUnrolling = CodeGenOpts.UnrollLoops;
   // For historical reasons, loop interleaving is set to mirror setting for loop
   // unrolling.
   PTO.LoopInterleaving = CodeGenOpts.UnrollLoops;
   PTO.LoopVectorization = CodeGenOpts.VectorizeLoop;
   PTO.SLPVectorization = CodeGenOpts.VectorizeSLP;
+  PTO.Coroutines = LangOpts.Coroutines;
 
   PassInstrumentationCallbacks PIC;
   StandardInstrumentations SI;
   SI.registerCallbacks(PIC);
   PassBuilder PB(TM.get(), PTO, PGOOpt, &PIC);
 
   // Attempt to load pass plugins and register their callbacks with PB.
   for (auto &PluginFN : CodeGenOpts.PassPlugins) {
if (CodeGenOpts.OptimizationLevel == 0) {
         PB.registerPipelineStartEPCallback([Options](ModulePassManager &MPM) {
           MPM.addPass(InstrProfiling(*Options, false));
         });
 
       if (IsThinLTO) {
         MPM = PB.buildThinLTOPreLinkDefaultPipeline(
             Level, CodeGenOpts.DebugPassManager);
         MPM.addPass(CanonicalizeAliasesPass());
         MPM.addPass(NameAnonGlobalPass());
       } else if (IsLTO) {
         MPM = PB.buildLTOPreLinkDefaultPipeline(Level,
                                                 CodeGenOpts.DebugPassManager);
         MPM.addPass(CanonicalizeAliasesPass());
         MPM.addPass(NameAnonGlobalPass());
       } else {
         MPM = PB.buildPerModuleDefaultPipeline(Level,
                                                CodeGenOpts.DebugPassManager);
       }
if (LangOpts.Sanitize.has(SanitizerKind::HWAddress)) {
           /*CompileKernel=*/false, Recover));
     }
     if (LangOpts.Sanitize.has(SanitizerKind::KernelHWAddress)) {
       MPM.addPass(HWAddressSanitizerPass(
           /*CompileKernel=*/true, /*Recover=*/true));
     }
 
     if (CodeGenOpts.OptimizationLevel == 0) {
+      addCoroutinePassesAtO0(MPM, LangOpts, CodeGenOpts);
       addSanitizersAtO0(MPM, TargetTriple, LangOpts, CodeGenOpts);
     }
   }
 
   // FIXME: We still use the legacy pass manager to do code generation. We
   // create that pass manager here and use it as needed below.
   legacy::PassManager CodeGenPasses;
   bool NeedCodeGen = false;

clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp

+// Tests that coroutine passes are added to and run by the new pass manager
+// pipeline, at -O0 and above.
+
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O0 %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm-bc -o /dev/null \
+// RUN:   -fexperimental-new-pass-manager -fdebug-pass-manager -fcoroutines-ts \
+// RUN:   -O1 %s 2>&1 | FileCheck %s
+//
+// CHECK: Starting llvm::Module pass manager run.
+// CHECK: Running pass:{{.*}}CoroEarlyPass
+//
+// The first coro-split pass enqueues a second run of the entire CGSCC pipeline.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// The second coro-split pass splits coroutine 'foo' into funclets
+// 'foo.resume', 'foo.destroy', and 'foo.cleanup'.
+// CHECK: Starting CGSCC pass manager run.
+// CHECK: Running pass: CoroSplitPass on (_Z3foov)
+// CHECK: Running pass:{{.*}}CoroElidePass{{.*}} on (_Z3foov)
+// CHECK: Finished CGSCC pass manager run.
+//
+// CHECK: Running pass:{{.*}}CoroCleanupPass
+// CHECK: Finished llvm::Module pass manager run.
+
+namespace std {
+namespace experimental {
+
+struct handle {};
+
+struct awaitable {
+  bool await_ready() { return true; }
+  void await_suspend(handle) {}
+  bool await_resume() { return true; }
+};
+
+template <typename T> struct coroutine_handle {
+  static handle from_address(void *address) { return {}; }
+};
+
+template <typename T = void> struct coroutine_traits {
+  struct promise_type {
+    awaitable initial_suspend() { return {}; }
+    awaitable final_suspend() { return {}; }
+    void return_void() {}
+    T get_return_object() { return T(); }
+    void unhandled_exception() {}
+  };
+};
+} // namespace experimental
+} // namespace std
+
+void foo() { co_return; }