[PATCH] D75440: [Coroutines] Optimized coroutine elision based on reachability

[Brian Gesiak via Phabricator via llvm-commits]

modocache accepted this revision.
modocache added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroElide.cpp:251
+        SwitchInst *SWI = cast<SwitchInst>(CSI->use_begin()->getUser());
+        // Only collect switchInsts that donot changed
+        if (SWI->getNumCases() == 2)
----------------
junparser wrote:
> modocache wrote:
> > 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.
> 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?).  
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.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75440/new/

https://reviews.llvm.org/D75440