This is an archive of the discontinued LLVM Phabricator instance.

[coroutines] Pass coro func args to promise ctor
ClosedPublic

Authored by modocache on Jan 8 2018, 6:15 AM.

Details

Summary

Use corutine function arguments to initialize a promise type, but only
if the promise type defines a constructor that takes those arguments.
Otherwise, fall back to the default constructor.

Test Plan: check-clang

Diff Detail

Repository
rL LLVM

Event Timeline

modocache created this revision.Jan 8 2018, 6:15 AM
GorNishanov requested changes to this revision.Jan 10 2018, 4:51 PM

Thank you for doing this change!

lib/Sema/SemaCoroutine.cpp
475 ↗(On Diff #128916)

I would keep this block of functions in their original place. (Or move them here as a separate NFC) commit.

Easier to review what was changed and what was simply moved.

570 ↗(On Diff #128916)

This creates a copy of the CoroutineParameterMoves map in every iteration of the loop.
It seems like an intent is just to create a shorter alias "Moves" to it to refer later.

I suggest:

  1. Make Moves a reference to the map: auto &Moves = ...
  2. Move it out of the loop
572 ↗(On Diff #128916)

Instead of doing lookup twice, once, using find, another, using operator[], you can just say:

auto EntryIter = Moves.find(PD);
if (EntryIter != Moves.end()) {
  auto *VD = cast<VarDecl>(cast<DeclStmt>(EntryIter->second)->getSingleDecl());
...
574 ↗(On Diff #128916)

This VD hides outer VD referring to the promise.
I would rename one of them to some other name.

619 ↗(On Diff #128916)

I believe this assert needs to be removed. We can get an invalid decl if we failed to find an appropriate constructor. (Couple of negative tests in SemaCXX/coroutines.cpp would help to flush those cases out)

test/CodeGenCoroutines/coro-alloc.cpp
196 ↗(On Diff #128916)

I would move this test to coro-params.cpp, as it is closer to parameter moves than to allocations.

I would also add a negative test or two to SemaCXX/coroutines.cpp to verify that we emit sane errors when something goes wrong with promise constructor and parameter copies.

This revision now requires changes to proceed.Jan 10 2018, 4:51 PM

Is this behavior specified somewhere? Or are we simply adding an extension to Clang? If so I would really prefer to add my co_promise solution (but I need to write a paper in favor of it first).

Is this behavior specified somewhere? Or are we simply adding an extension to Clang?

It is not specified anywhere yet but Gor has promised a paper for Jacksonville.

If so I would really prefer to add my co_promise solution (but I need to write a paper in favor of it first).

I have no problem with you making a separate proposal. I don't consider this an either/or thing.

Is this behavior specified somewhere? Or are we simply adding an extension to Clang? If so I would really prefer to add my co_promise solution (but I need to write a paper in favor of it first).

Before bringing the language change proposal, it makes sense to implement the feature and test it out on real examples. MSVC compiler implemented a number of coroutine features to do sanity testing and put in the customers hands before a feature was proposed.

Since this is a non-breaking change, I think it makes sense to put it in and play with it. This particular approach was talked about since 2014, but, no formal proposal was made. I am bringing the paper to upcoming Jacksonville meeting and would like to make sure that we have both implementation and usage experience for the feature.

modocache updated this revision to Diff 129785.Jan 14 2018, 9:53 AM

Thanks for the great review, @GorNishanov! You were exactly right, I had to remove the assert. I've taken all of your other suggestions as well. Let me know if anything else stands out at you. Also, thanks for the question, @EricWF, I added some comments to make it clear that this is an implementation of an experimental feature that has yet to be formally proposed.

modocache marked 5 inline comments as done.Jan 14 2018, 9:57 AM
This revision is now accepted and ready to land.Jan 23 2018, 3:59 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.

Great, thanks for the reviews!