[Coroutine] Make dealing with alloca spills more robust

[lxfind]

Summary

D66230 attempted to fix a problem where when there are allocas used before CoroBegin.
It keeps allocas and their uses stay in put if there are no escapse/changes to the data before CoroBegin.
Unfortunately that's incorrect.
Consider this code:

%var = alloca i32
%1 = getelementptr .. %var; stays put
%f = call i8* @llvm.coro.begin
store ... %1
After this fix, %1 will now stay put, however if a store happens after coro.begin and hence modifies the content, this change will not be reflected in the coroutine frame (and will eventually be DCEed).
To generalize the problem, if any alias ptr is created before coro.begin for an Alloca and that alias ptr is latter written into after coro.begin, it will lead to incorrect behavior.

There are also a few other minor issues, such as incorrect dominate condition check in the ptr visitor, unhandled memory intrinsics and etc.
Ths patch attempts to fix some of these issue, and make it more robust to deal with aliases.

While visiting through the alloca pointer, we also keep track of all aliases created that will be used after CoroBegin. We track the offset of each alias, and then reacreate these aliases after CoroBegin using these offset.
It's worth noting that this is not perfect and there will still be cases we cannot handle. I think it's impractical to handle all cases given the current design.
This patch makes it more robust and should be a pure win.
In the meantime, we need to think about what how to completely elimiante these issues, likely through the route as @rjmccall mentioned in D66230.

[lxfind]

Add comments

[lxfind]

Amend description

[lxfind]

Add reviewers

[lxfind]

llvm/test/Transforms/Coroutines/ArgAddr.ll

50

Looks like this is incorrect. It doesn't work well when CoroSaves are in loops. I will look into this.

[lxfind]

Use a different approach

[lxfind]

Update description

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

923–924

If there is a case:

%a = alloca..
%b = bitcast  %a to ...
@coro.begin
// some use of b but no use of a

in this case, all the use of %a is dominated by Coro.begin, so the variable MightNeedToCopy may not be true, then the codes before won't be executed.

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

639

Would there be cast* or GEP instruction of Alloca before Coro.Begin? If it is, it seems like we need to visit all the use of Alloca in AllocaVisitor

[lxfind]

@ChuanqiXu Thank you for taking a look!

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

639

Yes that's exactly what we are doing in the visitor.

923–924

MightNeedToCopy will be true if any use if the AllocaInst is not dominated by coro.begin. So in your example, since %b uses %a and happens before coro.begin, it's not dominated by coro.begin, and hence MightNeedToCopy will be true.

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

923–924

Yes, you are right.

[ChuanqiXu]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

639

Sorry, I made a mistake. It is right.

[lxfind]

address lint

[hoy]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

624–625

typo : pointing

630–634

This does sound fragile. I'm wondering if this is a defined behavior in the language standard. For a local variable first allocated on the local frame and later on copied into the coroutine frame, all subsequent accesses to it should be redirected to the coroutine frame? If so, can the coroutine frame be allocated ealier?

[lxfind]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

This is defined behavior in the language. How to manage the data in the heap is implementation details and should not affect program correctness. So it's certainly true that this implementation is not quite right yet. The hard part is that some optimization passes will insert allocas before coro.begin, which makes it difficult to track. A proper implementation should provide strong guarantees.
This patch only makes existing algorithm slightly more robust, but does not solve the more fundamental problem, which will take some time to design and implement.

[hoy]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

I see. Is it possible to fix those passes allocating locals before coro.begin? If that's too involved, can we move the coro.begin to the beginning of a function and redirect all local references to the coro frame? It's basically similar to outline all the function code except for the coro.begin call.

[lxfind]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

The problem is that coro.begin depends on the creation of the frame on the heap, and the creation of the heap sometimes could depend on parameters of the function, which will lead to allocas before coro.begin.
I will need to look into how this is done in the Swift ABI and potentially use that model.

[modimo]

Looking at https://bugs.llvm.org/show_bug.cgi?id=36578:
Andrew Kelley 2019-08-11 09:00:59 PDT
I'm no longer subscribed to this bug report. I've come to the conclusion that LLVM's coroutine API is not worth using, and resorting to implementing coroutines directly in the frontend.

That's an option to look into for solutions here. Also official docs for coro.begin: https://llvm.org/docs/Coroutines.html#llvm-coro-begin-intrinsic

Having anything emitted before frame-setup is tricky because any operation there needs to be tied into DWARF unwind codes. Mirroring these operations as the current design is doing may be a better solution given that. The assertion will definitely be a bug farm as every corner gets tested.

[hoy]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

How does the frame creation depend on the function local variable values? Does the frame size depend on the values ?

[lxfind]

In D86859#2255833, @modimo wrote:

Looking at https://bugs.llvm.org/show_bug.cgi?id=36578:
Andrew Kelley 2019-08-11 09:00:59 PDT
I'm no longer subscribed to this bug report. I've come to the conclusion that LLVM's coroutine API is not worth using, and resorting to implementing coroutines directly in the frontend.

That's an option to look into for solutions here. Also official docs for coro.begin: https://llvm.org/docs/Coroutines.html#llvm-coro-begin-intrinsic

Having anything emitted before frame-setup is tricky because any operation there needs to be tied into DWARF unwind codes. Mirroring these operations as the current design is doing may be a better solution given that. The assertion will definitely be a bug farm as every corner gets tested.

Yeah I will talk to Bruno latter and see what he thinks.

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

@hoy My understanding is that you could provide a custom Allocator for the frame creation, which comes from a parameter, which will be moved to a local variable the first thing in the function.

[hoy]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

I see. Thanks for the explanation. That sounds like a special use of locals which should have a very short lifetime. For regular local variables, their computation may be able to be deferred until they are allocated on the coroutine frame.

[lxfind]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

I agree. All the cases I have seen so far are Allocas generated for parameters, and there are only simple casts/GEPs happening before coro.begin. So I hope this patch is good for a while, and give us sometime to think about this more systematically.

[wenlei]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

can we move the coro.begin to the beginning of a function and redirect all local references to the coro frame

This seems promising to me. Relying on pointer not escaping data flow analysis for correctness is a bit scary.

We could have something like a canonicalization for the sequence of frame allocation, followed by coro.begin and then user alloca. The idea is still similar to D66230, but without changing the order of the interfering def-use.

[hoy]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

624–625

If the use is the pointer, should the store be rewritten if it is dominated by coro.begin?

637

Is a normal function call needed to be handled?

638

Does a load from an alloca need to be rewritten?

[hoy]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

Yeah, still trying to figure out what locals could be used before coro.begin. Are the lifetimes used by the coro.begin call all compiler-generated, not user code? If so, the compiler can ensure that all locals are allocated on the coroutine frame before they are used.

[lxfind]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

624–625

We only visit instructions before coro.begin, so it won't be dominated by coro.begin.

637

It's already handled in the base class, where it assumes escape.

638

No because it would already have the right value.

[lxfind]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

@wenlei @hoy
The frame allocation (and hence coro.begin) is supposed to be the first thing that's happening in the coroutine function. However, there is one complication.
The frame allocation can use a user-defined placement form of operator new to allocate the memory, which can take arguments from the arguments list in the function (refer to https://en.cppreference.com/w/cpp/language/coroutines, the Heap Allocation section).
Allocas can then be introduced when trying to pass parameters of the function into the frame allocation call. When other optimization passes are involved, sometimes alias of the allocas for the parameters can be created which are used latter after coro.begin.
In theory, the frame allocation call can also have side-effects to the parameters, which are already stored in Allocas.
So in the current model without any major redesign, I don't think there is a perfect solution. We can only make a best-effort try to copy anything that might have been modified and recreate any pointer that points to the stack.
Assuming that all the complications are introduced due to parameter passing, I do think that the pointer analysis is powerful enough to handle all cases that's reasonable. But to make it truly robust, we would need to do something entirely different.

[hoy]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

So sounds like only parameter locations need to be seen by coro.begin. We might be able to allow special temporary allocas for the sake of coro.begin while allocating real parameter and other locals on the coroutine frame directly.

[hoy]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

Talked offline, current coro.begin intrinsic call doesn't kill values crossing it, thus it doesn't prevent optimization passes from setting a value before it and using the value after it. This is also not modeled by any IR intrinsic attribute either. I guess we can assign coro.begin an existing attribute to prevent any function call being moved across it to minimize the escaped case.

The patch looks a decent workaround to me without introducing fundamental changes to the current coroutine implementation model.

[hoy]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

624–625

Should we check if the use is the left-hand side or right-hand side?

[hoy]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

624–625

Ignore my last comment please. I'm wondering why it's changed from escaped to aborted.

[lxfind]

address typo

[hoy]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

624–625

So why do we change from setAborted to setEscaped?

[lxfind]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

624–625

setAborted will cause the visiting process to terminate right after this instruction, which was fine in the previous implementation because all it needed was to find out whether it escapes or gets modified. But now I also need to track all aliases, which mean I have to visit every instruction and cannot abort in the middle.

[wenlei]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

The frame allocation can use a user-defined placement form of operator new to allocate the memory, which can take arguments from the arguments list in the function

Ok, so having some sequence before coro.begin may be required for frame allocation on heap via custom allocator. But that isn't problematic by itself, as long as nothing from frame allocation will cross coro.begin, right? And frame allocation should be a well structured sequence, wondering what would cause something to be used by both frame allocation and after coro.begin?

For this case below you mentioned in comment, assuming neither %a nor %b is used in frame allocation, would moving coro.begin above them (but still below frame allocation sequence) solve the problem? I can see that your fix makes things better, but trying to understand its overlap/tradeoff against the move approach (improving Gor's original fix to avoid breaking def-use chain).

%a = AllocaInst ...
%b = call @computeAddress(... %a)

[lxfind]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

So far the only problematic cases I know of are when the parameters of the function are localized though AllocaInst before coro.begin, and then used after coro.begin. This can happen when the custom allocator needs to use those parameters or some other optimization passes inserted AllocaInst in the front for whatever reason.
I don't actually think the counter-example in my comment (%b = call @computeAddress(... %a)) would really happen today, otherwise this solution won't work. I put down the comment just to note what the current solution cannot cover in case we hit it in the future. If this does happen, it would still most likely be due to the custom allocator (say the allocation call is inlined, and latter some optimization pass makes some of the local variables to be shared across coro.begin boundary)

[lxfind]

rebase before land

[hoy]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

I'm also wondering maybe we should assert for the cases we've not seen and cannot handle so far, i.e., escaped local addresses before coro.begin.

[lxfind]

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

630–634

I did consider this, but decided not to, because it's very likely the escape analysis in LLVM is not good enough to tell what could really escape. For instance, the custom allocator function might not be marked as non-escaping even if it won't escape.

[wenlei]

Chatted with @lxfind off patch in the morning. A quick summary:

We could still move instructions as much as we can after coro.begin, but defs used by both heap allocation and after coro.begin will still necessitate the need for alias tracking for things that cannot be moved.

However on top of the alias tracking implemented in this patch, moving things after coro.begin could handle some corner cases that alias tracking may fail. Let's have this in first, and we can add the instruction moving as an optimization later.

Thanks for working on this!

llvm/lib/Transforms/Coroutines/CoroFrame.cpp

Shape.RetconLowering.IsFrameInlineInStorage
       = (B.getStructSize() <= Id->getStorageSize() &&
          B.getStructAlign() <= Id->getStorageAlignment());
     break;
   }
   }
 
   return FrameTy;
 }
 
-// We use a pointer use visitor to discover if there are any writes into an
-// alloca that dominates CoroBegin. If that is the case, insertSpills will copy
-// the value from the alloca into the coroutine frame spill slot corresponding
-// to that alloca.
-namespace {
-struct AllocaUseVisitor : PtrUseVisitor<AllocaUseVisitor> {
-  using Base = PtrUseVisitor<AllocaUseVisitor>;
-  AllocaUseVisitor(const DataLayout &DL, const DominatorTree &DT,
-                   const CoroBeginInst &CB)
-      : PtrUseVisitor(DL), DT(DT), CoroBegin(CB) {}
-
-  // We are only interested in uses that dominate coro.begin.
-  void visit(Instruction &I) {
-    if (DT.dominates(&I, &CoroBegin))
-      Base::visit(I);
-  }
-  // We need to provide this overload as PtrUseVisitor uses a pointer based
-  // visiting function.
-  void visit(Instruction *I) { return visit(*I); }
-
-  void visitLoadInst(LoadInst &) {} // Good. Nothing to do.
-
-  // If the use is an operand, the pointer escaped and anything can write into
-  // that memory. If the use is the pointer, we are definitely writing into the
-  // alloca and therefore we need to copy.
-  void visitStoreInst(StoreInst &SI) { PI.setAborted(&SI); }
-
-  // Any other instruction that is not filtered out by PtrUseVisitor, will
-  // result in the copy.
-  void visitInstruction(Instruction &I) { PI.setAborted(&I); }
-
-private:
-  const DominatorTree &DT;
-  const CoroBeginInst &CoroBegin;
-};
-} // namespace
-static bool mightWriteIntoAllocaPtr(AllocaInst &A, const DominatorTree &DT,
-                                    const CoroBeginInst &CB) {
-  const DataLayout &DL = A.getModule()->getDataLayout();
-  AllocaUseVisitor Visitor(DL, DT, CB);
-  auto PtrI = Visitor.visitPtr(A);
-  if (PtrI.isEscaped() || PtrI.isAborted()) {
-    auto *PointerEscapingInstr = PtrI.getEscapingInst()
-                                     ? PtrI.getEscapingInst()
-                                     : PtrI.getAbortingInst();
-    if (PointerEscapingInstr) {
-      LLVM_DEBUG(
-          dbgs() << "AllocaInst copy was triggered by instruction: "
-                 << *PointerEscapingInstr << "\n");
-    }
-    return true;
-  }
-  return false;
-}
-
 // We need to make room to insert a spill after initial PHIs, but before
 // catchswitch instruction. Placing it before violates the requirement that
 // catchswitch, like all other EHPads must be the first nonPHI in a block.
 //
 // Split away catchswitch into a separate block and insert in its place:
 //
 //   cleanuppad <InsertPt> cleanupret.
 //
 // cleanupret instruction will act as an insert point for the spill.
 static Instruction *splitBeforeCatchSwitch(CatchSwitchInst *CatchSwitch) {
   BasicBlock *CurrentBlock = CatchSwitch->getParent();
   BasicBlock *NewBlock = CurrentBlock->splitBasicBlock(CatchSwitch);
   CurrentBlock->getTerminator()->eraseFromParent();
 
   auto *CleanupPad =
       CleanupPadInst::Create(CatchSwitch->getParentPad(), {}, "", CurrentBlock);
   auto *CleanupRet =
       CleanupReturnInst::Create(CleanupPad, NewBlock, CurrentBlock);
   return CleanupRet;
 }
 
+// Find the nearest Terminator instruction that dominates all CoroSave
+// instructions.
+static Instruction *findDominatorOfAllCoroSaves(coro::Shape &Shape) {
+  Instruction *Dom{nullptr};
+  DominatorTree DT(*Shape.CoroBegin->getFunction());
+  for (const AnyCoroSuspendInst *I : Shape.CoroSuspends) {
+    CoroSaveInst *CSI = I->getCoroSave();
+    if (!Dom) {
+      // Each CoroSaveInst should be in a separate BB due to earlier
+      // simplifications, hence it's guaranteed to have a single
+      // predecessor.
+      Dom = CSI->getParent()->getSinglePredecessor()->getTerminator();
+      continue;
+    }
+    BasicBlock *BB =
+        DT.findNearestCommonDominator(Dom->getParent(), CSI->getParent());
+    if (BB == CSI->getParent())
+      Dom = CSI->getParent()->getSinglePredecessor()->getTerminator();
+    else if (BB != Dom->getParent())
+      Dom = BB->getTerminator();
+  }
+  return Dom;
+}
+
 // Replace all alloca and SSA values that are accessed across suspend points
 // with GetElementPointer from coroutine frame + loads and stores. Create an
 // AllocaSpillBB that will become the new entry block for the resume parts of
 // the coroutine:
 //
 //    %hdl = coro.begin(...)
 //    whatever
 //
for (auto &P : Allocas) {
       // here, as we are changing location of the instruction.
       G->takeName(P.first);
       P.first->replaceAllUsesWith(G);
       P.first->eraseFromParent();
     }
     return FramePtr;
   }
 
-  // If we found any alloca, replace all of their remaining uses with GEP
-  // instructions. Because new dbg.declare have been created for these alloca,
-  // we also delete the original dbg.declare and replace other uses with undef.
-  // Note: We cannot replace the alloca with GEP instructions indiscriminately,
-  // as some of the uses may not be dominated by CoroBegin.
-  bool MightNeedToCopy = false;
+  SuspendCrossingInfo Checker(*CB->getFunction(), Shape);
+  SmallVector<Instruction *, 4> UsersAfterSuspend;
+  SmallVector<Instruction *, 4> UsersAfterCoroBegin;
+  auto UpdateUsersWithGEP = [&](auto &P, auto &UsersToUpdate) {
     Builder.SetInsertPoint(&Shape.AllocaSpillBlock->front());
-  SmallVector<Instruction *, 4> UsersToUpdate;
+    auto *G = GetFramePointer(P.second, P.first);
+    G->takeName(P.first);
+    for (Instruction *I : UsersToUpdate)
+      I->replaceUsesOfWith(P.first, G);
+  };
   for (auto &P : Allocas) {
     AllocaInst *const A = P.first;
 
     for (auto *DI : FindDbgDeclareUses(A))
       DI->eraseFromParent();
     replaceDbgUsesWithUndef(A);
 
-    UsersToUpdate.clear();
+    UsersAfterSuspend.clear();
+    UsersAfterCoroBegin.clear();
+    bool HasUsersBeforeCoroBegin = false;
     for (User *U : A->users()) {
       auto *I = cast<Instruction>(U);
-      if (DT.dominates(CB, I))
-        UsersToUpdate.push_back(I);
+      // TODO: All arguments should be passed as const.
+      if (Checker.isDefinitionAcrossSuspend(*P.first, U))
+        UsersAfterSuspend.push_back(I);
+      else if (DT.dominates(CB, I))
+        UsersAfterCoroBegin.push_back(I);
       else
-        MightNeedToCopy = true;
-    }
-    if (!UsersToUpdate.empty()) {
-      auto *G = GetFramePointer(P.second, A);
-      G->takeName(A);
-      for (Instruction *I : UsersToUpdate)
-        I->replaceUsesOfWith(A, G);
+        HasUsersBeforeCoroBegin = true;
     }
+    if (!UsersAfterSuspend.empty())
+      UpdateUsersWithGEP(P, UsersAfterSuspend);
+    if (!HasUsersBeforeCoroBegin) {
+      UpdateUsersWithGEP(P, UsersAfterCoroBegin);
+      continue;
     }
-  // If we discovered such uses not dominated by CoroBegin, see if any of them
-  // preceed coro begin and have instructions that can modify the
-  // value of the alloca and therefore would require a copying the value into
-  // the spill slot in the coroutine frame.
-  if (MightNeedToCopy) {
-    Builder.SetInsertPoint(FramePtr->getNextNode());
 
-    for (auto &P : Allocas) {
-      AllocaInst *const A = P.first;
-      if (mightWriteIntoAllocaPtr(*A, DT, *CB)) {
-        if (A->isArrayAllocation())
-          report_fatal_error(
-              "Coroutines cannot handle copying of array allocas yet");
+    Instruction *Dom = findDominatorOfAllCoroSaves(Shape);
+    Builder.SetInsertPoint(Dom);
 
-        auto *G = GetFramePointer(P.second, A);
-        auto *Value = Builder.CreateLoad(A->getAllocatedType(), A);
-        Builder.CreateStore(Value, G);
-      }
-    }
-  }
+
   return FramePtr;
 }
 
 // Sets the unwind edge of an instruction to a particular successor.
 static void setUnwindEdgeTo(Instruction *TI, BasicBlock *Succ) {
   if (auto *II = dyn_cast<InvokeInst>(TI))
     II->setUnwindDest(Succ);
   else if (auto *CS = dyn_cast<CatchSwitchInst>(TI))

llvm/test/Transforms/Coroutines/ArgAddr.ll

 ; Need to move users of allocas that were moved into the coroutine frame after
 ; coro.begin.
 ; RUN: opt < %s -preserve-alignment-assumptions-during-inlining=false -O2 -enable-coroutines -S | FileCheck %s
 ; RUN: opt < %s -preserve-alignment-assumptions-during-inlining=false  -aa-pipeline=basic-aa -passes='default<O2>' -enable-coroutines -S | FileCheck %s
 
 define nonnull i8* @f(i32 %n) {
 entry:
   %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null);
   %n.addr = alloca i32
-  store i32 %n, i32* %n.addr ; this needs to go after coro.begin
+  store i32 %n, i32* %n.addr
   %0 = tail call i32 @llvm.coro.size.i32()
   %call = tail call i8* @malloc(i32 %0)
   %1 = tail call noalias nonnull i8* @llvm.coro.begin(token %id, i8* %call)
   %2 = bitcast i32* %n.addr to i8*
   call void @ctor(i8* %2)
   br label %for.cond
 
 for.cond:

 entry:
   %hdl = call i8* @f(i32 4)
   call void @llvm.coro.resume(i8* %hdl)
   call void @llvm.coro.resume(i8* %hdl)
   call void @llvm.coro.destroy(i8* %hdl)
   ret i32 0
 ; CHECK:      call void @ctor
 ; CHECK-NEXT: call void @print(i32 4)
+; CHECK-NEXT: call void @llvm.lifetime.end.p0i8(i64 4, i8* nonnull %0)
 ; CHECK-NEXT: call void @print(i32 3)
 ; CHECK-NEXT: call void @print(i32 2)
 ; CHECK:      ret i32 0
 }
 
 declare i8* @malloc(i32)
 declare void @free(i8*)
 declare void @print(i32)

llvm/test/Transforms/Coroutines/coro-frame-arrayalloca.ll

suspend:
   ret i8* %hdl
 }
 
 ; See if the array alloca was stored as an array field.
 ; CHECK-LABEL: %f.Frame = type { void (%f.Frame*)*, void (%f.Frame*)*, double, double, [4 x i32], i1 }
 
 ; See if we used correct index to access prefix, data, suffix (@f)
 ; CHECK-LABEL: @f(
-; CHECK:       %prefix = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 2
-; CHECK-NEXT:  %data = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 4
-; CHECK-NEXT:  %suffix = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 3
+; CHECK:       %suffix = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 3
+; CHECK-NEXT:  %data = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 4, i32 0
+; CHECK-NEXT:  %prefix = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 2
 ; CHECK-NEXT:  call void @consume.double.ptr(double* %prefix)
 ; CHECK-NEXT:  call void @consume.i32.ptr(i32* %data)
 ; CHECK-NEXT:  call void @consume.double.ptr(double* %suffix)
 ; CHECK: ret i8*
 
 ; See if we used correct index to access prefix, data, suffix (@f.resume)
 ; CHECK-LABEL: @f.resume(
 ; CHECK:       %[[SUFFIX:.+]] = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 3

llvm/test/Transforms/Coroutines/coro-param-copy.ll

 ; Check that we create copy the data from the alloca into the coroutine
 ; frame slot if it was written to.
 ; RUN: opt < %s -coro-split -S | FileCheck %s
 ; RUN: opt < %s -passes=coro-split -S | FileCheck %s
 
 define i8* @f() "coroutine.presplit"="1" {
 entry:
+  %a.addr = alloca i64 ; used only after coro.begin.
+
   %x.addr = alloca i64
-  call void @use(i64* %x.addr) ; might write to %x
+  call void @use(i64* %x.addr) ; uses %x.addr before coro.begin.
+
   %y.addr = alloca i64
-  %y = load i64, i64* %y.addr ; cannot modify the value, don't need to copy
-  call void @print(i64 %y)
+  %y.cast = bitcast i64* %y.addr to i8* ; alias created and used after coro.begin.
+  
+  %z.addr = alloca i64
+  %flag = call i1 @check()
+  br i1 %flag, label %flag_true, label %flag_merge
+
+flag_true:
+  call void @use(i64* %z.addr) ; conditionally used %z.addr.
+  br label %flag_merge
 
+flag_merge:
   %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
   %size = call i32 @llvm.coro.size.i32()
-  %alloc = call i8* @myAlloc(i64 %y, i32 %size)
+  %alloc = call i8* @myAlloc(i64* %y.addr, i32 %size)
   %hdl = call i8* @llvm.coro.begin(token %id, i8* %alloc)
+  call void @use(i64* %a.addr)
+  call void @use(i64* %x.addr)
+  call void @llvm.memset.p0i8.i32(i8* %y.cast, i8 1, i32 4, i1 false)
+  call void @use(i64* %z.addr)
   %0 = call i8 @llvm.coro.suspend(token none, i1 false)
   switch i8 %0, label %suspend [i8 0, label %resume
                                 i8 1, label %cleanup]
 resume:
+  call void @use(i64* %a.addr)
   call void @use(i64* %x.addr)
   call void @use(i64* %y.addr)
+  call void @use(i64* %z.addr)
   br label %cleanup
 
 cleanup:
   %mem = call i8* @llvm.coro.free(token %id, i8* %hdl)
   call void @free(i8* %mem)
   br label %suspend
 suspend:
   call i1 @llvm.coro.end(i8* %hdl, i1 0)
   ret i8* %hdl
 }
 
-; See that we added both x and y to the frame.
-; CHECK: %f.Frame = type { void (%f.Frame*)*, void (%f.Frame*)*, i64, i64, i1 }
+; See that we added a, x, y and z to the frame.
+; CHECK: %f.Frame = type { void (%f.Frame*)*, void (%f.Frame*)*, i64, i64, i64, i64, i1 }
 
 ; See that all of the uses prior to coro-begin stays put.
 ; CHECK-LABEL: define i8* @f() {
 ; CHECK-NEXT: entry:
-; CHECK-NEXT:   %x.addr = alloca i64
+; CHECK-NEXT:   %x.addr = alloca i64, align 8
+; CHECK-NEXT:   call void @use(i64* %x.addr)
+; CHECK-NEXT:   %y.addr = alloca i64, align 8
+; CHECK-NEXT:   %y.cast = bitcast i64* %y.addr to i8*
+; CHECK-NEXT:   %z.addr = alloca i64, align 8
+; CHECK-NEXT:   %flag = call i1 @check()
+; CHECK-NEXT:   br i1 %flag, label %flag_true, label %flag_merge
+
+; CHECK:        call void @use(i64* %z.addr)
+; CHECK-NEXT:   br label %flag_merge
+
+; See that all uses of x, y and z will stay put, but in the end they are all copied into the frame.
+; a will not be copied since it wasn't touched before coro.begin.
+; CHECK:        %a.addr = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 2
+; CHECK-NEXT:   call void @use(i64* %a.addr)
 ; CHECK-NEXT:   call void @use(i64* %x.addr)
-; CHECK-NEXT:   %y.addr = alloca i64
-; CHECK-NEXT:   %y = load i64, i64* %y.addr
-; CHECK-NEXT:   call void @print(i64 %y)
-
-; See that we only copy the x as y was not modified prior to coro.begin.
-; CHECK:  store void (%f.Frame*)* @f.destroy, void (%f.Frame*)** %destroy.addr
-; CHECK-NEXT:  %0 = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 2
-; CHECK-NEXT:  %1 = load i64, i64* %x.addr
-; CHECK-NEXT:  store i64 %1, i64* %0
-; CHECK-NEXT:  %index.addr1 = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 4
-; CHECK-NEXT:  store i1 false, i1* %index.addr1
-; CHECK-NEXT:  ret i8* %hdl
+; CHECK-NEXT:   call void @llvm.memset.p0i8.i32(i8* %y.cast, i8 1, i32 4, i1 false)
+; CHECK-NEXT:   call void @use(i64* %z.addr)
+; CHECK-NEXT:   %0 = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 3
+; CHECK-NEXT:   %1 = load i64, i64* %x.addr, align 4
+; CHECK-NEXT:   store i64 %1, i64* %0, align 4
+; CHECK-NEXT:   %2 = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 4
+; CHECK-NEXT:   %3 = load i64, i64* %y.addr, align 4
+; CHECK-NEXT:   store i64 %3, i64* %2, align 4
+; CHECK-NEXT:   %4 = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 5
+; CHECK-NEXT:   %5 = load i64, i64* %z.addr, align 4
+; CHECK-NEXT:   store i64 %5, i64* %4, align 4
 
 declare i8* @llvm.coro.free(token, i8*)
 declare i32 @llvm.coro.size.i32()
 declare i8  @llvm.coro.suspend(token, i1)
 declare void @llvm.coro.resume(i8*)
 declare void @llvm.coro.destroy(i8*)
 
 declare token @llvm.coro.id(i32, i8*, i8*, i8*)
 declare i1 @llvm.coro.alloc(token)
 declare i8* @llvm.coro.begin(token, i8*)
 declare i1 @llvm.coro.end(i8*, i1)
 
-declare noalias i8* @myAlloc(i64, i32)
-declare void @print(i64)
+declare void @llvm.memset.p0i8.i32(i8*, i8, i32, i1)
+
+declare noalias i8* @myAlloc(i64*, i32)
 declare void @use(i64*)
 declare void @free(i8*)
+declare i1 @check()