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

[JunMa via Phabricator via llvm-commits]

junparser marked an inline comment as done.
junparser added a comment.

In D75440#1904205 <https://reviews.llvm.org/D75440#1904205>, @modocache wrote:

> > 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.



================
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)
----------------
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?).  


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

https://reviews.llvm.org/D75440