[PATCH] D55966: Ensure coro split pass only spills variables dominated by CoroBegin

[Brian Gesiak via Phabricator via llvm-commits]

modocache requested changes to this revision.
modocache added a comment.
This revision now requires changes to proceed.

Excellent progress! Thanks for working on this.

However I don't think this logic isn't quite right. My understanding is that in the context of coroutines transforms, the term "spill" refers to the definition of a value appearing before a suspend point, and a use of that value appearing after. All coroutines include an implicit suspend point at their start and end: the `promise_type.initial_suspend` and `.final_suspend` points. So any argument passed into a coroutine and then used from within its body does in fact "spill". For example:

  task<int> foo(int x) {
    // Implicit initial suspend point here.
    int y = x + 1; // The definition of 'x' comes at the start of the function, and its use is here past the implicit initial suspend point, so 'x' "spills" across a suspend point.
    // ...
  }

Because `x` spills, the coroutine transform passes move it onto the coroutine frame. This ensures that the value of `x` is preserved across suspend points.

The logic in this patch, however, prevents `x` in the example above from being considered a "spill." As a result, it is not moved onto the coroutine frame, and so its value is not preserved.

You can see this in practice by looking at the sample program in https://godbolt.org/z/YNX4dS, which I'll also paste below:

  #include <experimental/coroutine>
  #include <iostream>
  
  struct task {
    class promise_type;
    using handle_type = std::experimental::coroutine_handle<promise_type>;
  
    struct promise_type {
      task get_return_object() { return task{handle_type::from_promise(*this)}; }
      std::experimental::suspend_always initial_suspend() { return {}; }
      std::experimental::suspend_always final_suspend() { return {}; }
      void return_void() {}
      void unhandled_exception() { std::exit(1); }
  
      void *operator new(size_t sz, int x) {
        std::cout << __PRETTY_FUNCTION__ << " -> " << x << std::endl;
        return malloc(sz);
      }
    };
  
    handle_type handle = nullptr;
  
    explicit task(handle_type handle) : handle(handle) {}
    task(task&& t) : handle(t.handle) {}
    ~task() { if (handle) handle.destroy(); }
    void resume() { handle.resume(); }
  };
  
  task foo(int x) {
    std::cout << __PRETTY_FUNCTION__ << " -> " << x << std::endl;
    co_return;
  }
  
  int main() {
    task t = foo(42);
    t.resume();
  }

At `-O0` the program compilation succeeds, and when run it produces this output:

  static void *task::promise_type::operator new(size_t, int) -> 42
  task foo(int) -> 42

At `-O1` the program produces the error described in https://bugs.llvm.org/show_bug.cgi?id=36578. With your patch applied, it no longer triggers the LLVM coroutines assert, but the value of `x` is lost:

  static void *task::promise_type::operator new(size_t, int) -> 42
  task foo(int) -> 0

So while this patch avoids the assert, it doesn't fix the underlying issue: that `x` spills and so needs to be stored on the coroutine frame, but it cannot be moved onto the coroutine frame because it is *also* used in the allocator of the frame. I'm not sure what the ideal solution here is... perhaps `x` needs to be duplicated as `x.allocarg` -- `x.allocarg` would be passed into the allocator and would not spill, whereas `x` would continue to be used in the body of the function and would be spilled (and could be moved past and dominated by `@llvm.coro.begin`). But I am not certain this would work in practice -- maybe @GorNishanov has thought about this problem some more since we last spoke?



================
Comment at: lib/Transforms/Coroutines/CoroFrame.cpp:870
+  for (User *U : V.users())
+    if (cast<Instruction>(U) && !DT.dominates(&CBInst, cast<Instruction>(U)))
+      return false;
----------------
I believe `llvm::cast<T>` asserts if the cast to `T` fails, so this would assert for users that are not `Instruction`, such as `Constant`. In the future I'd recommend using `dyn_cast`, since it returns a false-y value if the cast fails:

```
if (auto *I = dyn_cast<Instruction>(U)) {
  if (!DT.dominates(..., I) {
    ...
  }
}
```

Writing it this way would also remove the need for the second `cast`.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55966/new/

https://reviews.llvm.org/D55966