[PATCH] D89768: [Coroutine] Properly determine whether an alloca should live on the frame

Mon Oct 26 23:36:44 PDT 2020

junparser added a comment.

In D89768#2354119 <https://reviews.llvm.org/D89768#2354119>, @lxfind wrote:

> In D89768#2353488 <https://reviews.llvm.org/D89768#2353488>, @junparser wrote:
>
>> Thanks for the patch! I think I generally agree with this patch. 
>> One thing is that the aliases seems to be over-analyzed. Can we just leave aliases on frame? Cause I have no idea about this effect on real workloads.
>
> What do you mean by "leave aliases on frame"?

Just setEscaped for pointer cast.  or can we do something like  llvm::PointerMayBeCaptured/llvm::PointerMayBeCapturedBefore to check the pointer directly? I'm not sure about this.

================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:919-921
+    for (auto *BB1 : UserBBs)
+      for (auto *BB2 : UserBBs)
+        if (Checker.hasPathCrossingSuspendPoint(BB1, BB2))
----------------
lxfind wrote:
> junparser wrote:
> > this function implies dominate relation for arg1 and arg2, so we can not use hasPathCrossingSuspendPoint for two user basic blocks directly. 
> hmm if that's the case, I think we might already have some buggy code here. The checks on lifetime markers don't necessary have dominance relationships. I saw you removed the dominance assertion in https://reviews.llvm.org/D75664. Do you remember why?
The bitcast users hit the assertion, however, they do not across the suspend point due to rewriteMaterializableInstructions. Except that, each lifetime marker has dominance relationship with some of the users. Iterate all the lifetime marker has effect as same as def 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89768