Page MenuHomePhabricator

[Coroutines] Run coroutine passes by default
ClosedPublic

Authored by ChuanqiXu on Jul 13 2021, 1:21 AM.

Details

Summary

Background: https://groups.google.com/g/llvm-dev/c/_Np-EWe662Q

Simply, before this patch, following command would fail:

clang -emit-llvm  -c    -O3  -Xclang -disable-llvm-passes \
          clang/test/CodeGenCoroutines/coro-symmetric-transfer-01.cpp \
         -std=c++20 -o coro.bc
clang -c coro.bc

First, I think it should be a defect that we need to fix. Since the IR generated by disable-llvm-passes should be valid IR and should be valid input to clang.
Then I think there would be two options:

  • Add an option in clang like the option -enable-coroutines in opt.
  • Make coroutine passes run by default in LLVM pipeline.

This patch implements the second methods. On the one hand, the coroutine passes seems to be stable since there are already many projects using coroutine feature. On the other hand, the coroutine passes should do nothing for IR who doesn't contain coroutine intrinsic.

So I think it should be OK to turn coroutine passes by default.

Test Plan: check-llvm

Diff Detail

Event Timeline

ChuanqiXu created this revision.Jul 13 2021, 1:21 AM
ChuanqiXu requested review of this revision.Jul 13 2021, 1:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2021, 1:21 AM
ChuanqiXu updated this revision to Diff 358214.Jul 13 2021, 3:09 AM

Refine test for new-pm-O0-defaults.ll

can you add a clang test to make sure the commands in the description work?
are the coroutine passes basically free when coroutines are not present? (i.e. they do a quick check and bail out immediately)

lxfind added inline comments.Jul 13 2021, 8:49 AM
llvm/lib/Transforms/Utils/InlineFunction.cpp
2194

What's the context of this change? Doesn't seem to be related to this diff?

aeubanks added inline comments.Jul 13 2021, 8:56 AM
llvm/lib/Passes/PassBuilder.cpp
1934

is this not an issue anymore?

ChuanqiXu updated this revision to Diff 358495.Jul 13 2021, 7:37 PM

Add test to address comments.

can you add a clang test to make sure the commands in the description work?

Done.

are the coroutine passes basically free when coroutines are not present? (i.e. they do a quick check and bail out immediately)

When coroutines are not present, the coroutine passes would check if the current function contains coroutine intrinsic. If no, coroutine passes would exit quickly. I think the check process is quick enough.

llvm/lib/Passes/PassBuilder.cpp
1934

Yes, I handled this in InlineFunction.cpp

llvm/lib/Transforms/Utils/InlineFunction.cpp
2194

Related contexts are at line 1934 in PassBuilder.cpp. The pass builder would set InsertLifetimeIntrinsics to be true for AlwaysInliner even at O0 as the comments tells.

This change would make sure the AlwaysInliner to insert lifetimes at O0. It wouldn't affect inliner since inliner wouldn't run at O0. And normal inliner would insert lifetime all the time.

I think I'd slightly prefer a separate option rather than always running coroutine passes, but we already have OpenMP passes in the pipeline that always run, so this isn't any worse than that, so this seems fine

llvm/lib/Transforms/Utils/InlineFunction.cpp
2194

can you add this to the description? or even better, split it out into a different patch?

ChuanqiXu updated this revision to Diff 358816.Jul 14 2021, 7:21 PM

Address comments.

ChuanqiXu added inline comments.Jul 14 2021, 7:24 PM
llvm/lib/Transforms/Utils/InlineFunction.cpp
2194

I added this to the comment. It is a little odd for me to split it into another patch. Since it means that we need to revert a change first (Insert lifetime for coroutine for always inliner at O0) and recommit it later. But from my point of view, it doesn't need to revert it first.

lxfind accepted this revision.Jul 14 2021, 9:53 PM

LGTM

This revision is now accepted and ready to land.Jul 14 2021, 9:53 PM
aeubanks accepted this revision.Jul 14 2021, 10:22 PM
This revision was landed with ongoing or failed builds.Jul 14 2021, 11:34 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 14 2021, 11:34 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nikic added a subscriber: nikic.Aug 3 2021, 12:37 PM

I noticed that this change had a measurable impact on O0 memory usage, which I wouldn't have expected (https://llvm-compile-time-tracker.com/compare.php?from=0f9e6451a836886f39137818c4f0cfd69ae31e62&to=8a1727ba51d262365b0d9fe10fef7e50da7022cd&stat=max-rss). Any idea what could cause it? Some additional analysis results hanging around?

lxfind added a comment.Aug 3 2021, 1:21 PM

I noticed that this change had a measurable impact on O0 memory usage, which I wouldn't have expected (https://llvm-compile-time-tracker.com/compare.php?from=0f9e6451a836886f39137818c4f0cfd69ae31e62&to=8a1727ba51d262365b0d9fe10fef7e50da7022cd&stat=max-rss). Any idea what could cause it? Some additional analysis results hanging around?

That's surprising. Is there a way to measure these benchmarks locally? We could probably find out which one is causing the issue by manually commenting out each coro pass and see how the number changes.

it's probably because we're constructing the call graph every time at -O0

we want to skip all of that if there are no coroutine intrinsics in the module. we can't really express that currently. if we'd want to do something like that, we'd need a wrapper around the passes which checks for the existence of those intrinsics before running the passes, including the CGSCC adaptor

I noticed that this change had a measurable impact on O0 memory usage, which I wouldn't have expected (https://llvm-compile-time-tracker.com/compare.php?from=0f9e6451a836886f39137818c4f0cfd69ae31e62&to=8a1727ba51d262365b0d9fe10fef7e50da7022cd&stat=max-rss). Any idea what could cause it? Some additional analysis results hanging around?

CoroElide would require Alias Analysis and DominatorTreeAnalysis. Before I didn't noticed that since CoroElide would only happen if inline happens. I would try to add guard by optimization level for CoroElide pass.

I noticed that this change had a measurable impact on O0 memory usage, which I wouldn't have expected (https://llvm-compile-time-tracker.com/compare.php?from=0f9e6451a836886f39137818c4f0cfd69ae31e62&to=8a1727ba51d262365b0d9fe10fef7e50da7022cd&stat=max-rss). Any idea what could cause it? Some additional analysis results hanging around?

CoroElide would require Alias Analysis and DominatorTreeAnalysis. Before I didn't noticed that since CoroElide would only happen if inline happens. I would try to add guard by optimization level for CoroElide pass.

Sorry I found that CoroElide wouldn't run at O0. So the reason should be the construction for call graph as @aeubanks said.