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

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

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.



================
Comment at: clang/docs/ClangCommandLineReference.rst:1580
+
+Enable support for P2014R0 Option2, which will always pursue the aligned allocation function.
+This option is disabled by default.
----------------



================
Comment at: clang/docs/ReleaseNotes.rst:152-154
+- Implemented `-fcoro-aligned-allocation` flag. This option2 implement
+  option2 for P2014R0 aligned allocation of coroutine frames
+  (`P2014R0 <https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/p2014r0.pdf>`_).
----------------



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11283
+def err_conflicting_aligned_options : Error <
+  "conflicting option '-fcoro-aligned-allocation' and '-fno-aligned-allocation'"
 >;
----------------
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.


================
Comment at: clang/include/clang/Basic/LangOptions.def:157
 LANGOPT(Coroutines        , 1, 0, "C++20 coroutines")
+LANGOPT(CoroAlignedAllocation, 1, 0, "prefer Aligned Allocation according to P2014's option2")
 LANGOPT(DllExportInlines  , 1, 1, "dllexported classes dllexport inline methods")
----------------



================
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
----------------
Update comment 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
----------------
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?


================
Comment at: clang/test/CodeGenCoroutines/coro-aligned-alloc.cpp:94
+    void return_value(int) {}
+    void operator delete(void *ptr, std::align_val_t);
+  };
----------------
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.


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

https://reviews.llvm.org/D133341