[PATCH] D133341: [C++] [Coroutines] Prefer aligned (de)allocation for coroutines - implement the option2 of P2014R0

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu marked 2 inline comments as done.
ChuanqiXu added a comment.

In D133341#3788283 <https://reviews.llvm.org/D133341#3788283>, @ychen wrote:

> It surprises me that Option 2 does not change https://eel.is/c++draft/dcl.fct.def.coroutine#10. For consistency, I think it should. And according to your test case, it deals with alignment as expected. Probably we should change the P2014R0 wording accordingly. Before that, let's just mention this difference in the Clang release notes.

Yeah, in fact due to the wording of coroutine allocation has changed slightly recently, the wording of P2014 <https://reviews.llvm.org/P2014> must be updated before merged. And I agree it should be consistent.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11283
+def err_conflicting_aligned_options : Error <
+  "conflicting option '-fcoro-aligned-allocation' and '-fno-aligned-allocation'"
 >;
----------------
ychen wrote:
> Since we're digressing from the usual operator new/delete look-up rules per Option 2, I think it might be easier to just *not* respect `-fno-aligned-allocation` and `-fno-sized-deallocation`. Using '-fcoro-aligned-allocation' explicitly should permit us to assume these functions could be found.
I guess it may not be good to enable `-fsized-deallocation` automatically if `-fcoro-aligned-allocation` enabled. Since it looks like there are other reasons why we disabled `-fsized-deallocation` before. And it looks it will block our users to use `-fcoro-aligned-allocation`. For the `-faligned-allocation` flag, this is enabled by default but people could disable it explicitly. So the check here is for that.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1302-1318
   // According to [dcl.fct.def.coroutine]p9, Lookup allocation functions using a
   // parameter list composed of the requested size of the coroutine state being
   // allocated, followed by the coroutine function's arguments. If a matching
   // allocation function exists, use it. Otherwise, use an allocation function
   // that just takes the requested size.
   //
   // [dcl.fct.def.coroutine]p9
----------------
ychen wrote:
> Update comment here.
Since P2014 is not standardized, I feel it might not be good to edit them here.


================
Comment at: clang/lib/Sema/SemaCoroutine.cpp:1400
 
+  // If we found a non-aligned allocation function in the promise_type,
+  // it indicates the user forgot to update the allocation function. Let's emit
----------------
ychen wrote:
> Option 2's order of look-up is 
> ```
> void* T::operator new  ( std::size_t count, std::align_val_t al, user-defined-args... );
> void* T::operator new  ( std::size_t count, std::align_val_t al);
> void* T::operator new  ( std::size_t count, user-defined-args... );
> void* T::operator new  ( std::size_t count);
> void* operator new  ( std::size_t count, std::align_val_t al );
> ```
> Why not allow `void* T::operator new  ( std::size_t count);` here?
My original thought is to be as strict as we can. But now I feel a warning may be good enough.


================
Comment at: clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp:94
+    void return_value(int) {}
+    void operator delete(void *ptr, std::align_val_t);
+  };
----------------
ychen wrote:
> Please add test cases for 
> ```
> void operator delete  ( void* ptr, std::size_t, std::align_val_t);
> void operator delete  ( void* ptr, std::size_t);
> void operator delete  ( void* ptr);
> void ::operator delete  ( void* ptr, std::size_t, std::align_val_t);
> void ::operator delete  ( void* ptr, std::size_t);
> void ::operator delete  ( void* ptr);
> ```
> in that lookup order, and makes sure the look-up order is expected.
I've tried my best. But it looks hard to test these operator delete under global namespace since they are automatically declared by the compiler.


================
Comment at: clang/test/SemaCXX/coroutine-alloc-4.cpp:49
+    void return_value(int) {}
+    void *operator new(std::size_t, std::align_val_t) noexcept;
+    void *operator new(std::size_t) noexcept;
----------------
ychen wrote:
> Like this test case, please add additional test cases to check the expected look-up order, one test for each consecutive pair should be good.
> 
> ```
> void* T::operator new  ( std::size_t count, std::align_val_t al, user-defined-args... );
> void* T::operator new  ( std::size_t count, std::align_val_t al);
> void* T::operator new  ( std::size_t count, user-defined-args... );
> void* T::operator new  ( std::size_t count);
> void* operator new  ( std::size_t count, std::align_val_t al );
> ```
> 
> 
Yeah, I'm testing this in CodeGenCoroutines. (It is hard to test the selection in Sema Test)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133341/new/

https://reviews.llvm.org/D133341