Page MenuHomePhabricator

[Coroutines] Use allocator overload when available

Authored by modocache on Jan 26 2018, 3:30 PM.



Depends on

An implementation of the behavior described in [dcl.fct.def.coroutine]/7:
when a promise type overloads operator new using a "placement new"
that takes the same argument types as the coroutine function, that
overload is used when allocating the coroutine frame.

Simply passing references to the coroutine function parameters directly
to operator new results in invariant violations in LLVM's coroutine
splitting pass, so this implementation modifies Clang codegen to
produce allocator-specific alloc/store/loads for each parameter being
forwarded to the allocator.

Test Plan: check-clang

Diff Detail

rC Clang

Event Timeline

modocache created this revision.Jan 26 2018, 3:30 PM
modocache updated this revision to Diff 131729.Jan 28 2018, 1:29 PM

Add some diagnostics tests to SemaCXX/coroutines.cpp.

GorNishanov requested changes to this revision.Feb 1 2018, 4:57 PM
GorNishanov added inline comments.
526 ↗(On Diff #131729)

First, thank you for doing this change!

Second, I am not sure that this part (CGCoroutine.cpp change) belongs in clang.
llvm CoroFrame is doing an unsafe transformation (it was safe until we got the arguments to operator new :-) ).

Moving the stores after the new that loads from those stores is an incorrect transformation. I think it needs to be addressed in llvm. getNotRelocatableInstructions function in CoroSplit.cpp needs to add special handling for AllocaInst and freeze the stores to that Alloca in the blocks preceeding the one with CoroBegin (getCoroBeginPredBlocks).

Also, for this to work for cases where parameter is used both in allocating function and in the body of the coroutine we need to have a copy. Currently, front-end does not create copies for scalar types (see CoroutineStmtBuilder::makeParamMoves() in SemaCoroutine.cpp). I think if we always create copies for all parameters, it will make this change more straightforward.

Third, this code does not handle cases where scalar values passed by reference to an allocation function or a struct passed by value:

Try this code on these:

void *operator new(unsigned long, promise_matching_placement_new_tag,
                   int&, float, double);


struct Dummy { int x, y, z; ~Dummy(); };

struct std::experimental::coroutine_traits<void, promise_matching_placement_new_tag, Dummy&, float, double> {
   struct promise_type {
       void *operator new(unsigned long, promise_matching_placement_new_tag,
                   Dummy, float, double);

I think if this code is changed according to my earlier suggestion of doing copies in clang and freezing stores in the llvm, it should take care the cases above.

These cases need to be added as tests to llvm\tests\Transforms\Coroutines

This revision now requires changes to proceed.Feb 1 2018, 4:57 PM
GorNishanov added inline comments.Feb 1 2018, 5:37 PM
526 ↗(On Diff #131729)

Alternatively, we can keep current clang behavior with respect to not doing copies for scalars and instead create a copy in llvm if we decided to freeze the store AND that alloca is used after a suspend point (CoroFrame decided that it needs to spill it into the coroutine frame).

modocache updated this revision to Diff 133129.Feb 6 2018, 6:27 PM

Prevent coroutine parameter stores from being moved, rely on instead.

modocache updated this revision to Diff 133154.Feb 6 2018, 11:28 PM

Update the tests to work with scalar parameter copies.

GorNishanov added inline comments.Feb 13 2018, 9:49 AM

This does not implement TS specified behavior for non static member functions:

[dcl.fct.def.coroutine]/7 states that an argument list is build up as follows:

The first argument is the amount of space requested, and has type std::size_t. The lvalues p1 ... pn are the succeeding arguments.

Where p1 ... pn are defined earlier in

[dcl.fct.def.coroutine]/3 as:

For a coroutine f that is a non-static member function, let P1 denote the type of the implicit object parameter (13.3.1) and P2 ... Pn be the types of the function parameters; otherwise let P1 ... Pn be the types of the function parameters.

Essentially for non-static member functions, we need to insert implicit object parameter.

Note that lookupPromiseType in SemaCoroutine.cpp when building specialization of std::experimental::coroutine_traits<...> includes implicit object parameter:

// If the function is a non-static member function, add the type
// of the implicit object parameter before the formal parameters.
if (auto *MD = dyn_cast<CXXMethodDecl>(FD)) {
  if (MD->isInstance()) {
    // [over.match.funcs]4
    // For non-static member functions, the type of the implicit object
    // parameter is
    //  -- "lvalue reference to cv X" for functions declared without a
    //      ref-qualifier or with the & ref-qualifier
    //  -- "rvalue reference to cv X" for functions declared with the &&
    //      ref-qualifier
    QualType T =
    T = FnType->getRefQualifier() == RQ_RValue
            ? S.Context.getRValueReferenceType(T)
            : S.Context.getLValueReferenceType(T, /*SpelledAsLValue*/ true);
GorNishanov added inline comments.Feb 13 2018, 9:51 AM

I think promise constructor argument preview is also missing the implicit object parameter.

modocache updated this revision to Diff 134304.Feb 14 2018, 1:34 PM

Thanks for the review, @GorNishanov, and for pointing out that part of the standard! I added a reference to the implicit object parameter, as well as some tests for that behavior. And thanks for pointing out the flaw in -- I'll submit a separate diff to forward the implicit object parameter to promise type constructors.

modocache marked an inline comment as done.Feb 14 2018, 1:35 PM
This revision is now accepted and ready to land.Feb 15 2018, 9:44 AM
This revision was automatically updated to reflect the committed changes.

Hooray, [dcl.fct.def.coroutine]/7 is *in!*

Thank you for all the reviews, @GorNishanov!