[PATCH] D103593: [Coroutine] Sink lifetime markers after switch of suspend blocks to avoid disturbing must tail calls

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

In D103593#2833787 <https://reviews.llvm.org/D103593#2833787>, @lxfind wrote:

> On the other hand, I don't know what kind of lifetime analysis is out there that could be used by CoroSplit and is good enough.
> I wonder how bad it would be if we just disable/remove the `sinkLifetimeStartMarkers` step for now, and focus on finding a better lifetime analysis framework that would give us what we want?

To my knowledge, there isn't analysis/transformation available for our purpose now (reduce the lifetime range marked by lifetime markers). There is a pass named StackColoring, which could reduce the lifetime for stack variables. But we can't use it directly since they are implemented for MIR.

---

To summarize up, the solutions we found currently are:

1. Sink lifetime markers after the switch statement followed by 'coro.suspend' intrinsics. One key point we discussed for this solution is whether we should add guard codes to check the pattern. And if yes, how do we check the pattern?
2. Erase all the lifetime markers for the allocas we put on the frame after we made splitting. @rjmccall provided.
  1. Try to remove the lifetime markers when we convert tail call to musttail call at the end of Coro-split pass.
3. Remove sinkLifetimeStartMarkers simply and try to make a better lifetime analysis. @lxfind provided.

There are my thoughts about them. 
For the first: I prefer this method. Here are my reasons:

1. The algorithm of `sinkLifetimeStartMarkers` looks like right and it works well for a relative long time. We hadn't find bug reports for it for a while.
2. The problem we found now is caused by sinking lifetime start markers between 'coro.suspend' and the switch statement, which would break the transformation to the symmetric transfer.

So it looks like a minor issue of `sinkLifetimeStartMarkers` and we only need to sink lifetime start markers to the place where wouldn't break the transformation. And the third question is:

1. Is the new place we choose good?

I think the answer is yes. Since we didn't change the logic of `sinkLifetimeStartMarkers`. And if `sinkLifetimeStartMarkers ` is right, the codes after patching should be right too. Otherwise we should fix `sinkLifetimeStartMarkers `.
Another things to diccuss is whether to add check-codes. I think we can omit it and I said the reasons in the above.

For the second solution: Erase all the lifetime markers for the allocas we put on the frame after we made splitting.
The worries I got is that if the coroutine got elided and the frame got SROA, then the lifetime markers could be useful. But we had erased them.
The positive side is that this is simple to implement and the downside looks strict to trigger.

For the third solution: Try to remove the lifetime markers when we convert tail call to musttail call at the end of Coro-split pass
It looks like the second solution, which makes the condition to erase the lifetime markers more strict.

For the final solution:  Remove sinkLifetimeStartMarkers simply and try to make a better lifetime analysis.
It consists of two steps:

1. Remove sinkLifetimeStartMarkers.
2. Make a better and formal lifetime analysis.

The first step could fix the bug clearly. But we need to answer how much regression we would got if we removed this optimization. It's a hard question. It reveals that we lack the benchmark for coroutine again. BTW, I had tried to make a benchmark. But I find it's much much harder than I think. Maybe we could discuss this in another thread.
For the second step,  it should be harder to add a separate pass. But I agree with that we can improve the current analysis further more.

Overall,  I prefer the first solution since the downside is relatively minimal from my perspective. The second solution is also acceptable to me since the condition to trigger the bad results are rare and the bad behavior is not so bad. If we really care it, we could choose the third solution.
For the final solution, I think the better order should be 'Make a better lifetime analysis' and 'Replace `sinkLifetimeStartMarkers` with the new one'. Although we can't know exactly the impact of removing `sinkLifetimeStartMarkers `, it doesn't make sense to me that we remove an optimization arbitrarily. I agree with that we could and should improve `sinkLifetimeStartMarkers `, but I think we could make it in the future instead of this patch (It seems like the TODO list is already long though)


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

https://reviews.llvm.org/D103593