[PATCH] D23245: [Coroutines] Part 6: Elide dynamic allocation of a coroutine frame when possible

[David Majnemer via llvm-commits]

majnemer added inline comments.

================
Comment at: lib/Transforms/Coroutines/CoroElide.cpp:90-94
@@ +89,7 @@
+// See if any operand of the call instruction references the coroutine frame.
+static bool operandReferences(CallInst *CI, AllocaInst *Frame, AAResults &AA) {
+  for (Value *Op : CI->operand_values())
+    if (AA.alias(Op, Frame) != NoAlias)
+      return true;
+  return false;
+}
----------------
This doesn't seem conservative enough because you are only check if the call's operands alias the alloca.  I could imagine the call referencing the coroutine frame via other means (load/store via global).

I think `getModRefInfo() != MRI_NoModRef` would capture this better.

================
Comment at: lib/Transforms/Coroutines/CoroElide.cpp:103-105
@@ +102,5 @@
+  for (Instruction &I : instructions(F))
+    if (auto *Call = dyn_cast<CallInst>(&I))
+      if (Call->isTailCall() && operandReferences(Call, Frame, AA))
+        Call->setTailCall(false);
+}
----------------
Should we `report_fatal_error` if the `CallSite` is `must_tail`? 

================
Comment at: lib/Transforms/Coroutines/CoroElide.cpp:110
@@ +109,3 @@
+static Type *getFrameType(Function *Resume) {
+  auto ArgType = Resume->getArgumentList().front().getType();
+  return cast<PointerType>(ArgType)->getElementType();
----------------
`auto *`

================
Comment at: lib/Transforms/Coroutines/CoroElide.cpp:129
@@ +128,3 @@
+
+  auto *Frame = new AllocaInst(FrameTy, "", InsertPt);
+  auto *FrameVoidPtr =
----------------
Do we have to worry about the alignment of this alloca?

================
Comment at: lib/Transforms/Coroutines/CoroElide.cpp:194-195
@@ +193,4 @@
+
+  // If llvm.coro.begin refers to llvm.coro.alloc, we can elide the allocation.
+  auto *AllocInst = CoroBegin->getAlloc();
+
----------------
majnemer wrote:
> We would typically sink this assignment into the if.
Is this still safe if there are multiple coro.begin calls and some of them refer to llvm.coro.alloc and others do not?

================
Comment at: lib/Transforms/Coroutines/CoroElide.cpp:194-203
@@ +193,12 @@
+
+  // If llvm.coro.begin refers to llvm.coro.alloc, we can elide the allocation.
+  auto *AllocInst = CoroBegin->getAlloc();
+
+  // FIXME: Do more sophisticated check for when we can do heap elision.
+  // Something like: for every exit from the function where coro.begin is live,
+  // there is a coro.free or coro.destroy that dominates that exit block.
+  // At the moment we simply assume that if we found at least one coro.destroy
+  // referencing the coro.begin, we can elide the heap allocation.
+
+  if (AllocInst) {
+    auto FrameTy = getFrameType(cast<Function>(ResumeAddrConstant));
----------------
We would typically sink this assignment into the if.

================
Comment at: lib/Transforms/Coroutines/CoroElide.cpp:204
@@ +203,3 @@
+  if (AllocInst) {
+    auto FrameTy = getFrameType(cast<Function>(ResumeAddrConstant));
+    elideHeapAllocations(CoroBegin, FrameTy, AllocInst, AA);
----------------
`auto *`


https://reviews.llvm.org/D23245