Page MenuHomePhabricator

[Coroutines] Also check lifetime intrinsic for local variable when build coroutine frame
ClosedPublic

Authored by junparser on Mar 4 2020, 11:52 PM.

Details

Summary

Currently we move all allocas into the frame when build coroutine frame in CoroSplit pass. However, this can be relaxed.

Since CoroSplit pass run after Inline pass, we can use lifetime intrinsic to do such analysis: If the scope of lifetime intrinsic is
not across any suspend point, rather than move the allocas to frame, we can just move them to entry bb of corresponding function. This reduce the frame size.

More importantly, this also avoid data race in multithread environment. Consider one inline function by coroutine: it starts a thread which access local variables, while after inline the movement of allocs to frame also access them. cause data race.

TestPlan: check-llvm, cppcoro

Diff Detail

Event Timeline

junparser created this revision.Mar 4 2020, 11:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2020, 11:52 PM
wenlei added a comment.Mar 5 2020, 8:45 AM

Nice change, thanks @junparser! The improved dataflow analysis using lifetime instead of allocation point and the resulting shrink-wrapping style opt for alloca would definitely be beneficial.

For data race however, I'm not sure if I understand it correctly, could you elaborate (perhaps with a concrete example..) why this is compiler's fault?

Nice change, thanks @junparser! The improved dataflow analysis using lifetime instead of allocation point and the resulting shrink-wrapping style opt for alloca would definitely be beneficial.

For data race however, I'm not sure if I understand it correctly, could you elaborate (perhaps with a concrete example..) why this is compiler's fault?

For

auto await_suspend(std::experimental::coroutine_handle<> h) {
    auto& pr = h.promise();
    if (pr._ex) {
        // schedule to another thread
        pr._ex->schedule([&pr]() {
            pr._continuation.resume();
        });
    }
}

This case shows data race with inline, and does not when not inlined. It looks like both the current thread and schedule thread access some fields of std::function when I dig into the ir. I'm not sure whether this is caused by std::function constructor/destructor or move allocas to frame. I get such conclusion based on ir.

Nice change, thanks @junparser! The improved dataflow analysis using lifetime instead of allocation point and the resulting shrink-wrapping style opt for alloca would definitely be beneficial.

For data race however, I'm not sure if I understand it correctly, could you elaborate (perhaps with a concrete example..) why this is compiler's fault?

For

auto await_suspend(std::experimental::coroutine_handle<> h) {
    auto& pr = h.promise();
    if (pr._ex) {
        // schedule to another thread
        pr._ex->schedule([&pr]() {
            pr._continuation.resume();
        });
    }
}

This case shows data race with inline, and does not when not inlined. It looks like both the current thread and schedule thread access some fields of std::function when I dig into the ir. I'm not sure whether this is caused by std::function constructor/destructor or move allocas to frame. I get such conclusion based on ir.

More specifically, the tsan gives 'use after free' error. So I think tsan can trace such access of frame block, but it may not be able to trace local allocas.

It seems that this patch also fix some workaround in folly, https://github.com/facebook/folly/blob/master/folly/Portability.h#L501, This is the same behavior as well as our coroutine library

junparser added a comment.EditedMar 9 2020, 3:35 AM

It seems that this patch also fix some workaround in folly, https://github.com/facebook/folly/blob/master/folly/Portability.h#L501, This is the same behavior as well as our coroutine library

@wenlei, according to @lewissbaker 's comment on await object:

So within the await_suspend() method, once it’s possible for the coroutine to be resumed concurrently on another thread, you need to make sure that you avoid accessing this or the coroutine’s .promise() object because both could already be destroyed. In general, the only things that are safe to access after the operation is started and the coroutine is scheduled for resumption are local variables within await_suspend().

when local variable becomes frame variable, data race may happens.

wenlei accepted this revision.Mar 9 2020, 8:45 AM

Thanks for clarification, LGTM!

This revision is now accepted and ready to land.Mar 9 2020, 8:45 AM
junparser updated this revision to Diff 249300.EditedMar 10 2020, 3:44 AM

@wenlei @modocache would you please review the updated patch?
I make the update for :

  1. remove assertion since we use lifetime intrinsic as def
  2. support multiple lifetime intrinsic
  3. it invalidates one case in cppcoro (large number of synchronous completions doesn't result in stack-overflow) since we do not move local variables into frame now @lewissbaker would please udpate it?
modocache accepted this revision.Mar 13 2020, 5:51 AM

Nice, thanks for this improvement! Am I right in thinking patch is dependent upon D76119? My understanding is that, without that patch, then we wouldn't be able to rely on lifetime intrinsics when using -O0. You may want to use Phabricator's "Edit Related Objects" link to specify that this patch depends on that one.

This LGTM assuming we can rely on lifetime intrinsics being generated at -O0. I'll comment on D76119 on that point.

llvm/lib/Transforms/Coroutines/CoroFrame.cpp
1470

Nit-pick: I think the LLVM coding style would have you remove these for loop braces. There's only a single statement in the for loop, the if statement above, so you technically don't need these here.

llvm/lib/Transforms/Coroutines/CoroSplit.cpp
572

Wow, I'm surprised coro-split-02.ll was the only test case you ended up needing to modify. I would have thought that this one change would have broken a lot more tests' assertions.

584

Awesome! Nice to remove this TODO.

junparser marked an inline comment as done.Mar 15 2020, 6:11 PM

@modocache Thanks !

llvm/lib/Transforms/Coroutines/CoroFrame.cpp
1470

Thanks, I'll update this one.

junparser marked an inline comment as done.Mar 23 2020, 9:19 PM
junparser added inline comments.
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
1470

we should keep the braces, since there is another else below

This revision was automatically updated to reflect the committed changes.