[PATCH] D43000: [Coroutines] Don't move stores for allocator args

[Gor Nishanov via Phabricator via llvm-commits]

GorNishanov requested changes to this revision.
GorNishanov added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Transforms/Coroutines/CoroSplit.cpp:668
+        // by the coroutine frame allocator.
+        if (RelocBlocks.count(A->getParent()) != 0) {
+          for (const auto &User : A->users())
----------------
`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.



================
Comment at: lib/Transforms/Coroutines/CoroSplit.cpp:671
+            if (auto *SI = dyn_cast<llvm::StoreInst>(User))
+              DoNotRelocate.insert(SI);
+        }
----------------
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;
      }
```


Repository:
  rL LLVM

https://reviews.llvm.org/D43000