GorNishanov added inline comments.
================
Comment at: lib/CodeGen/CGCoroutine.cpp:526
EmitBlock(AllocBB);
- auto *AllocateCall = EmitScalarExpr(S.getAllocate());
+ // Emit the call to the coroutine frame allocation function.
+ auto *AllocateCall = cast<llvm::CallInst>(EmitScalarExpr(S.getAllocate()));
----------------
GorNishanov wrote:
> 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);
>
> or
>
> ```
> struct Dummy { int x, y, z; ~Dummy(); };
>
> template<>
> 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
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).
Repository:
rC Clang
https://reviews.llvm.org/D42606
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
