[coroutines] Insert spills of PHI instructions correctly

[GorNishanov]

Summary

Fix a bug where we were inserting a spill in between the PHIs in the beginning of the block.
Consider this fragment:

begin:
  %phi1 = phi i32 [ 0, %entry ], [ 2, %alt ]
  %phi2 = phi i32 [ 1, %entry ], [ 3, %alt ]
  %sp1 = call i8 @llvm.coro.suspend(token none, i1 false)
  switch i8 %sp1, label %suspend [i8 0, label %resume
                                  i8 1, label %cleanup]
resume:
  call i32 @print(i32 %phi1)

Unless we are spilling the argument or result of the invoke, we were always inserting the spill immediately following the instruction.
The fix adds a check that if the spilled instruction is a PHI Node, select an appropriate insert point with getFirstInsertionPt() that
skips all the PHI Nodes and EH pads.

[rnk]

lgtm

lib/Transforms/Coroutines/CoroFrame.cpp

441

This will be a problem if this phi is in a catchswitch block created for MSVC EH. Is that something you worry about yet? You can make such a phi like this:

int x = 0;
try {
  may_throw();
  ++x;
  may_throw();
} catch (...) {
  use_int(x);
}

I'm not sure what you need to do to make this a coroutine as well, but maybe you can make a test case that way.

Anyway, this is totally something that doesn't need to be addressed in this change.

[GorNishanov]

lib/Transforms/Coroutines/CoroFrame.cpp

441

Thank you very much for review!

Good point about cathswitch. I'll have a fix for it in two commits. I will need to add eh aware edge splitting first.

lib/Transforms/Coroutines/CoroFrame.cpp

if (CurrentValue != E.def()) {
           // the coroutine frame pointer instruction, i.e. bitcast of
           // coro.begin from i8* to %f.frame*.
           InsertPt = FramePtr->getNextNode();
         } else if (auto *II = dyn_cast<InvokeInst>(CurrentValue)) {
           // If we are spilling the result of the invoke instruction, split the
           // normal edge and insert the spill in the new block.
           auto NewBB = SplitEdge(II->getParent(), II->getNormalDest());
           InsertPt = NewBB->getTerminator();
+        } else if (dyn_cast<PHINode>(CurrentValue)) {
+          // Skip the PHINodes and EH pads instructions.
+          InsertPt =
+              &*cast<Instruction>(E.def())->getParent()->getFirstInsertionPt();
         } else {
           // For all other values, the spill is placed immediately after
           // the definition.
           assert(!isa<TerminatorInst>(E.def()) && "unexpected terminator");
           InsertPt = cast<Instruction>(E.def())->getNextNode();
         }
 
         Builder.SetInsertPoint(InsertPt);

test/Transforms/Coroutines/coro-spill-after-phi.ll

+; Verifies that we insert spills of PHI instruction _after) all PHI Nodes
+; RUN: opt < %s -coro-split -S | FileCheck %s
+
+define i8* @f(i1 %n) "coroutine.presplit"="1" {
+entry:
+  %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)
+  %size = call i32 @llvm.coro.size.i32()
+  %alloc = call i8* @malloc(i32 %size)
+  %hdl = call i8* @llvm.coro.begin(token %id, i8* %alloc)
+  br i1 %n, label %begin, label %alt
+alt:
+  br label %begin
+
+begin:
+  %phi1 = phi i32 [ 0, %entry ], [ 2, %alt ]
+  %phi2 = phi i32 [ 1, %entry ], [ 3, %alt ]
+
+  %sp1 = call i8 @llvm.coro.suspend(token none, i1 false)
+  switch i8 %sp1, label %suspend [i8 0, label %resume
+                                  i8 1, label %cleanup]
+resume:
+  call i32 @print(i32 %phi1)
+  call i32 @print(i32 %phi2)
+  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
+}
+
+; Verifies that the both phis are stored correctly in the coroutine frame
+; CHECK: %f.Frame = type { void (%f.Frame*)*, void (%f.Frame*)*, i1, i1, i32, i32 }
+; CHECK-LABEL: @f(
+; CHECK: store void (%f.Frame*)* @f.destroy, void (%f.Frame*)** %destroy.addr
+; CHECK: %phi1 = select i1 %n, i32 0, i32 2
+; CHECK: %phi2 = select i1 %n, i32 1, i32 3
+; CHECK: %phi2.spill.addr = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 5
+; CHECK: store i32 %phi2, i32* %phi2.spill.addr
+; CHECK: %phi1.spill.addr = getelementptr inbounds %f.Frame, %f.Frame* %FramePtr, i32 0, i32 4
+; CHECK: store i32 %phi1, i32* %phi1.spill.addr
+; CHECK: ret i8* %hdl
+
+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* @malloc(i32)
+declare i32 @print(i32)
+declare void @free(i8*)