Page MenuHomePhabricator

Ensure coro split pass only spills variables dominated by CoroBegin
AbandonedPublic

Authored by GorNishanov on Dec 20 2018, 3:42 PM.

Details

Reviewers
modocache
tks2103
Summary

Ensure coro split pass only spills variables dominated by CoroBegin

Fixes https://bugs.llvm.org/show_bug.cgi?id=36578.

The old coro split code didn't run safety checks against certain
spillable variables; it would try to spill things that were not
dominated by CoroBegin.

This problem gets worse after the mem2reg
pass is run, as the code which checks that Arguments are safe to
spill is even more lax than the code which checks that
Instructions are safe to spill.

There's a test which checks two things:

First, that a store/load optimized away by mem2reg doesn't fail.

Second, if there is an allocator which takes a potentially
spillable argument, we have to check all of the other arguments
to that allocator to ensure there are no unspillable instructions
as well.

Diff Detail

Event Timeline

tks2103 created this revision.Dec 20 2018, 3:42 PM
modocache requested changes to this revision.EditedDec 21 2018, 2:33 PM

Excellent progress! Thanks for working on this.

However I don't think this logic is 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?

lib/Transforms/Coroutines/CoroFrame.cpp
884

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.

This revision now requires changes to proceed.Dec 21 2018, 2:33 PM

Hmmm, duh.

Let me look at what's in the coroutine frame in the test case I have set up without running -mem2reg. If it's doing the right thing in that case, that'll provide a clue.

Creating copies for a scalar that is used in operator new and in the body of the function is a sound strategy.
Notice that if we replace int x parameter to Int x with the following int like class:

c++
struct Int {
  int value;
  Int(int v) : value(v) {}
  friend std::ostream& operator<<(std::ostream& o, const Int& me) {{
    return o << me.value;
  }}
  ~Int() { puts("destructor so that the Int is passed as UDT"); }
};

The problem goes away. It happens because operator new operates on raw parameters and the body operates on the copy of the parameters.
For scalar types, or on UDT types that can be turned into scalars after optimizations a copy of the parameter and the raw parameter is collapsed to the same value.

I think creating a copy of the scalar that has to go into the coroutine frame and that is used before and after coro.begin is sound, since it restores the property that was originally in the coroutine conceptual model and was optimized away in the passes prior to CoroSplit

tks2103 updated this revision to Diff 180732.Jan 8 2019, 1:49 PM

updated to handle case for a scalar spill Argument

GorNishanov requested changes to this revision.Jan 9 2019, 4:34 PM
GorNishanov added inline comments.
lib/Transforms/Coroutines/CoroFrame.cpp
932

This does not look right. It states that we never spill arguments that are pointer types, whether we use them across suspend point or not.

Any access to those arguments after suspend point will be garbage.

To observe, add i16* %x argument that you will use after suspend (either on resume path or destroy path` and observe that in .resume or .destroy parts of the coroutine any access to %x will be replaced with undef

This revision now requires changes to proceed.Jan 9 2019, 4:34 PM

Any update on this issue? We're trying to upgrade the tool chain for a substantial code base from clang 6.0 to a later version, and have been seeing this error starting with clang 7.0. It also appears in clang 8.0 and 9.0, so we're stuck making any progress to upgrade.

Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2019, 1:11 PM

Noted, I'll try to work on this issue in early August, at the latest. In the meantime feel free to claim that bug report and submit a patch. Unfortunately unless someone develops a patch and immediately requests it be merged into the LLVM 9.0.0 release branch, I don't think it'll be in 9.0.0.

Noted, I'll try to work on this issue in early August, at the latest. In the meantime feel free to claim that bug report and submit a patch. Unfortunately unless someone develops a patch and immediately requests it be merged into the LLVM 9.0.0 release branch, I don't think it'll be in 9.0.0.

Let me see if I can make the fix this week.

Noted, I'll try to work on this issue in early August, at the latest. In the meantime feel free to claim that bug report and submit a patch. Unfortunately unless someone develops a patch and immediately requests it be merged into the LLVM 9.0.0 release branch, I don't think it'll be in 9.0.0.

Let me see if I can make the fix this week.

Hopefully this week

GorNishanov commandeered this revision.Fri, Aug 16, 10:15 AM
GorNishanov edited reviewers, added: tks2103; removed: GorNishanov.

This is no longer needed. https://reviews.llvm.org/D66230 fixes the problem.

GorNishanov abandoned this revision.Fri, Aug 16, 10:16 AM