This is an archive of the discontinued LLVM Phabricator instance.

[Coroutines] Fix premature conversion of return object
ClosedPublic

Authored by bruno on Mar 8 2023, 6:53 PM.

Details

Summary

Fix https://github.com/llvm/llvm-project/issues/56532

Effectively, this reverts behavior introduced in https://reviews.llvm.org/D117087,
which did two things:

  1. Change delayed to early conversion of return object.
  2. Introduced RVO possibilities because of early conversion.

This patches fixes (1) and removes (2). I already worked on a follow up for (2)
in a separated patch. I believe it's important to split these two because if the RVO
causes any problems we can explore reverting (2) while maintaining (1).

Notes on some testcase changes:

  • pr59221.cpp changed to -O1 so we can check that the front-end honors the value checked for. Sounds like -O3 without RVO is more likely to work with LLVM optimizations...
  • Comment out delete members coroutine-no-move-ctor.cpp since behavior now requires copies again.

Diff Detail

Event Timeline

bruno created this revision.Mar 8 2023, 6:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 6:53 PM
Herald added subscribers: hoy, modimo, wenlei. · View Herald Transcript
bruno requested review of this revision.Mar 8 2023, 6:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2023, 6:53 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
weiwang added a subscriber: weiwang.Mar 8 2023, 8:31 PM
ChuanqiXu accepted this revision.Mar 9 2023, 1:37 AM

This is basically a reverting of https://reviews.llvm.org/D117087. So it should be good according to our previous talk.

clang/include/clang/AST/StmtCXX.h
410–414

Maybe we can delete this.

clang/lib/Sema/SemaCoroutine.cpp
1752

Let's remove this one.

This revision is now accepted and ready to land.Mar 9 2023, 1:37 AM
bruno added a comment.Mar 9 2023, 2:18 PM

Test failures are unrelated, thanks for the review @ChuanqiXu

This revision was landed with ongoing or failed builds.Mar 9 2023, 2:19 PM
This revision was automatically updated to reflect the committed changes.
bgraur added a subscriber: bgraur.Mar 15 2023, 9:37 AM

Reverting the RVO breaks some coroutine code we have. The short repro is https://gcc.godbolt.org/z/1543qc8Ee (code at the end of the post, very similar to the deleted constructor in warn-throw-out-noexcept-coro.cpp):
The RVO seems to be mandated by the standard and D145641 is in the works to fix this.

Could we revert this change and wait for D145641 to land before reverting existing RVO behavior?
This would unblock our compiler releases (we live at head). This looks like the right trade-off given that previous behavior was in Clang for more than a year after D117087 landed.

Reproducer (works in Clang 15 and GCC, fails in trunk):

#include <coroutine>

struct MyTask{
  struct promise_type {
    MyTask get_return_object() { return MyTask{std::coroutine_handle<promise_type>::from_promise(*this)}; }
    std::suspend_always initial_suspend() { return {}; }

    void unhandled_exception();
    void return_void() {} 

    auto await_transform(MyTask task) {
      struct Awaiter {
        bool await_ready() { return false; }
        std::coroutine_handle<promise_type> await_suspend(std::coroutine_handle<promise_type> h);
        std::nullptr_t await_resume();

        promise_type& caller;
        promise_type& callee;
      };

      return Awaiter{*this, task.handle.promise()};
    }
    
    auto final_suspend() noexcept {
      struct Awaiter {
        bool await_ready() noexcept { return false; }
        std::coroutine_handle<promise_type> await_suspend(std::coroutine_handle<promise_type> h) noexcept;
        void await_resume() noexcept;

        std::coroutine_handle<promise_type> to_resume;
      };

      return Awaiter{resume_when_done};
    }

    // The coroutine to resume when we're done.
    std::coroutine_handle<promise_type> resume_when_done;
  };

  explicit MyTask(std::coroutine_handle<promise_type>);
  MyTask(MyTask&&) = delete;

  // A handle for the coroutine that returned this task.
  std::coroutine_handle<promise_type> handle;
};

MyTask DoNothing() {
  co_return;
}

The reproducer seems like what we're searching for tests in https://reviews.llvm.org/D145641. Reverting is common in Clang/LLVM. But let's wait for the response from @bruno temporarily. I think it makes sense to revert this one if @bruno don't response tomorrow. And we can commit this one and D145641 quickly the next time. Recommit is common too.

The RVO seems to be mandated by the standard.

Yeah, I thought so... But WG21 decided to not make it in a recent meeting... So we can only make it as a non-mandated compiler optimization.

Reverting is common in Clang/LLVM. But let's wait for the response from @bruno temporarily. I think it makes sense to revert this one if @bruno don't response tomorrow. And we can commit this one and D145641 quickly the next time. Recommit is common too.

@bgraur, FYI, hope waiting for a day works for this case.

The RVO seems to be mandated by the standard.

Yeah, I thought so... But WG21 decided to not make it in a recent meeting... So we can only make it as a non-mandated compiler optimization.

That's unexpected. Are there any wording change proposals published?
Our understanding comes from the reading of dcl.fct.def.coroutine/7

The expression promise.get_return_object() is used to initialize the glvalue result or prvalue result object of a call to a coroutine.

And we think RVO is mandated when initializing the result objects mentioned in general.
Also, from https://github.com/llvm/llvm-project/issues/56532:

Q: if the initialization does occur later, by what mechanism the prvalue result of get_return_object is forwarded to that initialization.
A: (see below)
Case 1/2. Same type of get_return_object and coroutine return type.
Constructed in the return slot.
Case 3. Different types:
(1) Constructed temporary object prior to initial suspend initialized with a call to get_return_object()
(2) when coroutine needs to to return to the caller and needs to construct return value for the coroutine it is initialized with expiring value of the temporary obtained in step 1.

! In D145639#4198815, @ChuanqiXu wrote:
The RVO seems to be mandated by the standard.

Yeah, I thought so... But WG21 decided to not make it in a recent meeting... So we can only make it as a non-mandated compiler optimization.

That's unexpected. Are there any wording change proposals published?
Our understanding comes from the reading of dcl.fct.def.coroutine/7

The expression promise.get_return_object() is used to initialize the glvalue result or prvalue result object of a call to a coroutine.

And we think RVO is mandated when initializing the result objects mentioned in general.
Also, from https://github.com/llvm/llvm-project/issues/56532:

Q: if the initialization does occur later, by what mechanism the prvalue result of get_return_object is forwarded to that initialization.
A: (see below)
Case 1/2. Same type of get_return_object and coroutine return type.
Constructed in the return slot.
Case 3. Different types:
(1) Constructed temporary object prior to initial suspend initialized with a call to get_return_object()
(2) when coroutine needs to to return to the caller and needs to construct return value for the coroutine it is initialized with expiring value of the temporary obtained in step 1.

The formal file shouldn't be released yet. But we can find the issue report and the proposed solution in https://cplusplus.github.io/CWG/issues/2563.html.

The words you quoted is the recording of the wg21's discussion during the meeting. And that's the intention of the designers but it is not reflected to the wording. What issue 2563 discussed is that if we should delay the conversion. And the RVO is a conclusion of that. But according to the result, we should say the compiler is allowed to do RVO if the types matched but it is not required.

What's the resolution here? Can we revert this and continue the discussions independently?
We can always re-land this change if the conclusion is that the approach here is the one that we want.

I suspect D145641 won't land today in the EU working hours.
My suggestion would be to revert this patch and reland together with D145641.
This should be in line with @ChuanqiXu's suggestions to wait for a response today.

@bruno, @ChuanqiXu please let us know if you have any objections, otherwise we will land the revert in ~2 hours.

@bruno, @ChuanqiXu please let us know if you have any objections, otherwise we will land the revert in ~2 hours.

No sweat, I didn't see this in time. Thanks for the reduced testcase.

What's the resolution here? Can we revert this and continue the discussions independently?

Eager initialization breaks us, lack of RVO on matching types breaks others. Landing this + D145641 altogether seems like the best approach.

We can always re-land this change if the conclusion is that the approach here is the one that we want.

Once we wrap up D145641 I'll land both, as separated commits but in the same push.

ChuanqiXu reopened this revision.Mar 21 2023, 1:28 AM
This revision is now accepted and ready to land.Mar 21 2023, 1:28 AM
bruno updated this revision to Diff 507084.Mar 21 2023, 12:20 PM

Put dependency in place for D145641

This revision was automatically updated to reflect the committed changes.