[PATCH] D42606: [Coroutines] Use allocator overload when available

[Gor Nishanov via Phabricator via cfe-commits]

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