[Coroutines] Don't move stores for allocator args

[modocache]

Summary

The behavior described in Coroutines TS [dcl.fct.def.coroutine]/7
allows coroutine parameters to be passed into allocator functions.
The instructions to store values into the alloca'd parameters must not
be moved past the frame allocation, otherwise uninitialized values are
passed to the allocator.

Test Plan: check-llvm

[compnerd]

lib/Transforms/Coroutines/CoroSplit.cpp

669–674

Why not write this as:

for (const auto &User : Alloca->users())
  if (auto *SI = dyn_cast<llvm::StoreInst>(&User))
    DoNotRelocate.insert(SI);

[modocache]

Thanks, @compnerd! Updated to use your suggestion.

[modocache]

Added a test.

[GorNishanov]

lib/Transforms/Coroutines/CoroSplit.cpp

668

if (RelocBlocks.count(A->getParent()) != 0) {

I am wondering if this check necessary?

RelocBlocks contains the block with CoroBegin and all preceeding blocks.
We prime the work queue with CoroBegin and terminators of the preceeding blocks
All of the operands of instructions in the Work queue should be in the preceeding blocks.

I can imagine doing this check for SI, so that we will not place in DoNotRelocate StoreInst that may occur after Coro.Begin.

671

I do not think just DoNotRelocate.insert(SI) is sufficient. If we are freezing the store, we need to freeze the operands as well. So this part should look something like:

if (DoNotRelocate.count(I) == 0) {
  Work.push_back(I);
  DoNotRelocate.insert(I);
}

Consider this fragment (you probably won't be able to get it straight from clang, but, with some creative optimizer passes you may get something like this:

define i8* @f(i32 %argument) "coroutine.presplit"="1" {
entry:
  %argument.addr = alloca i32, align 4
  %x = add i32 %argument, 5 ; will get moved 
  store i32 %x, i32* %argument.addr, align 4 ; << frozen
  %id = call token @llvm.coro.id(i32 0, i8* null, i8* null, i8* null)

To fix, I believe your loop looking for stores needs to look like:

if (auto *A = dyn_cast<AllocaInst>(I)) {
  // Stores to alloca instructions that occur before coroutine frame
  // is allocated should not be moved; the stored values may be used
  // by the coroutine frame allocator.
  for (const auto &User : A->users())
    if (auto *SI = dyn_cast<llvm::StoreInst>(User))
      if (RelocBlocks.count(SI->getParent()) != 0 &&
          DoNotRelocate.count(SI) == 0) {
        Work.push_back(SI);
        DoNotRelocate.insert(SI);
      }
  continue;
}

[modocache]

Thanks for the review, @GorNishanov! I added something similar to your example of a store operand to the test case. Indeed, without the changes you proposed, the existence of the operand caused an invariant to be violated in LLVM. With your suggested changes, the assertion doesn't fire and the new test passes.

[GorNishanov]

LGTM!

[modocache]

Thank you, @GorNishanov!

lib/Transforms/Coroutines/CoroSplit.cpp

getNotRelocatableInstructions(CoroBeginInst *CoroBegin,
   for (BasicBlock *BB : RelocBlocks)
     if (BB != CoroBeginBB)
       Work.push_back(BB->getTerminator());
 
   // For every instruction in the Work list, place its operands in DoNotRelocate
   // set.
   do {
     Instruction *Current = Work.pop_back_val();
+    DEBUG(dbgs() << "CoroSplit: Will not relocate: " << *Current << "\n");
     DoNotRelocate.insert(Current);
     for (Value *U : Current->operands()) {
       auto *I = dyn_cast<Instruction>(U);
       if (!I)
         continue;
-      if (isa<AllocaInst>(U))
+
+      if (auto *A = dyn_cast<AllocaInst>(I)) {
+        // Stores to alloca instructions that occur before coroutine frame
+        // is allocated should not be moved; the stored values may be used
+        // by the coroutine frame allocator.
+        if (RelocBlocks.count(A->getParent()) != 0) {
+          for (const auto &User : A->users())
+            if (auto *SI = dyn_cast<llvm::StoreInst>(User))
+              DoNotRelocate.insert(SI);
+        }
         continue;
+      }
+
       if (DoNotRelocate.count(I) == 0) {
         Work.push_back(I);
         DoNotRelocate.insert(I);
       }
     }
   } while (!Work.empty());
   return DoNotRelocate;
 }