[PATCH] D91305: [Coroutine] Allocas used by StoreInst does not always escape

Fri Nov 13 20:54:39 PST 2020

junparser added a comment.

In D91305#2392970 <https://reviews.llvm.org/D91305#2392970>, @lxfind wrote:

> In D91305#2392934 <https://reviews.llvm.org/D91305#2392934>, @junparser wrote:
>
>> I'm not sure whether we should fix this case by case. Maybe we should use memoryssa to handle this.
>
> In general yes I agree with you that we don't want to handle case by case and make this over-complicated. However there are good reasons that this patch is quite necessary.
>
> 1. When optimizations are enabled, there are usually pretty accurate lifetime information, and hence we don't usually even need the alloca visitor to track them. So the alloca visitor is crucial when optimizations are turned off and there is no lifetime information. When optimizations are turned off, because there isn't mem2reg, almost all data accesses are done through the patterns of stores followed with loads. What I found is that in such case, when optimizations are turned off, almost all allocas are put on the frame because they are all used in store instructions, making this a bit useless. This patch is a key to make the alloca tracking effective and so that majority of allocas can be moved out of frame. Hence the effect of this patch is quite significant comparing to the effort. It's unlikely that I will add any more logic to track other cases.
> 2. This is also a continuation of D90990 <https://reviews.llvm.org/D90990>, which is to avoid frame-use-after-destroy. What I found is that, even I fixed the front-end such that the use of temporaries after a call to await_suspend() are contained locally that does not go across suspension points, because they are all involved in this store/load pattern, they are still being put on the frame, causing memory corruptions in non-optimized mode. This patch can fix that issue completely.

Do we really need handle this in non-optimized mode? @bruno I'm afraid that this patch may affect the debugging quality fixed in D90772 <https://reviews.llvm.org/D90772>, @lxfind, have you ever checked this?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91305