Page MenuHomePhabricator

[Coroutines] Optimized coroutine elision based on reachability
ClosedPublic

Authored by junparser on Mar 2 2020, 1:43 AM.

Details

Summary

For now, CoroElide pass can not handle case like:

coroA (cond){
   if (cond)
     co_await coroB
  else 
     co_await coroC
co_return
}

However, CoroElide should worked for this.

In this patch, If dominate relation is not satisfied, we iterate all paths from coro.begin to normal terminators to check whether there is any pathes from coro.begin to Terminators which not pass through any of the coro.destroys. If not, record the coro.begin.
As for the paths reach to normal terminators through suspend point, they have been skipped during DFS.

TestPlan: check-llvm, check-clang, cppcoro

Diff Detail

Event Timeline

junparser created this revision.Mar 2 2020, 1:43 AM

Thank you for this patch, I'll give it a proper review tonight. But in the meantime, I wonder if you've tried this with any of the sample programs for this bug report, https://bugs.llvm.org/show_bug.cgi?id=40656 ?

Thank you for this patch, I'll give it a proper review tonight. But in the meantime, I wonder if you've tried this with any of the sample programs for this bug report, https://bugs.llvm.org/show_bug.cgi?id=40656 ?

yep, I have tried the two samples, same behavior as clang6.

yep, I have tried the two samples, same behavior as clang6.

Phenomenal!! Thanks for checking. Please feel free to address the nit-pick comments only if you wish. I'll wait to accept until I better understand the rationale for checking the number of cases in a suspend switch statement, if that's alright.

llvm/lib/Transforms/Coroutines/CoroElide.cpp
33

A nit-pick, but: it's spelled "switches" with an "e". So I would rename this CoroSuspendSwitches, or if that's too long of a variable name for you, CoroSwitches. Also, this could be marked const to signal these won't be "lowered" or transformed in any way: SmallPtrSet<const SwitchInst *, 4>.

147

Another nit-pick: this can be const CoroBeginInst * (with a few modifications below, like SmallVector<const BasicBlock *, 32> Worklist;). I think it'd be a small improvement because it'd make clear that this function just analyzes paths through the IR, it doesn't modify it.

172

Yet another nit-pick, sorry: maybe it's just me but I would find this much clearer with --Limit; as its own statement on its own line. Just my opinion, though, please ignore it if you disagree.

259

Typo: "donot" should be "do not" -- but also, this is the first instance in which these coroutine passes check for the number of cases in a switch statement. Could you expand on this comment? I'm afraid I don't follow why coro.suspend with switches between resume/cleanup should be part of the path analysis, but others should not.

junparser marked an inline comment as done.Mar 3 2020, 5:47 PM

yep, I have tried the two samples, same behavior as clang6.

Phenomenal!! Thanks for checking. Please feel free to address the nit-pick comments only if you wish. I'll wait to accept until I better understand the rationale for checking the number of cases in a suspend switch statement, if that's alright.

Thanks for review the patch. I'll update the patch as suggestion.

llvm/lib/Transforms/Coroutines/CoroElide.cpp
259

According to https://llvm.org/docs/Coroutines.html, when FE create coro.suspend with switches, it should be :

%0 = call i8 @llvm.coro.suspend(token none, i1 false)
switch i8 %0, label %suspend [i8 0, label %resume
                              i8 1, label %cleanup]

The behavior of default dest is suspend the coroutine and return to caller which means this a escape path to normal terminator.

so for case like:

coroA  () {
     co_await coroB();
}

statement co_await coroB() is expanded by FE, and there is at least one coro.suspend between coro.begin and coro.destroy of coroB.

Skip the default dest of coro.suspend switches is reasonable since contents of coroutine frame doesn't change outside the coroutine body.

Btw, most of time the switches can not be simplified except final.suspend which we do not care (real terminator right?).

modocache accepted this revision.Mar 4 2020, 8:35 AM
modocache added inline comments.
llvm/lib/Transforms/Coroutines/CoroElide.cpp
259

Ah, I see! Very cool, thanks for the explanation, that helped me understand. (I think the comment could use an update based on what you wrote above.)

This revision is now accepted and ready to land.Mar 4 2020, 8:35 AM
junparser updated this revision to Diff 248397.Mar 4 2020, 10:33 PM

update the patch as suggestion

This revision was automatically updated to reflect the committed changes.