[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

[Xun Li via Phabricator via cfe-commits]

lxfind added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:2689
 
-  size_t __builtin_coro_size()
+  size_t __builtin_coro_size(bool alloc)
   void  *__builtin_coro_frame()
----------------
ChuanqiXu wrote:
> ychen wrote:
> > ChuanqiXu wrote:
> > > ychen wrote:
> > > > lxfind wrote:
> > > > > Do we need to change __builtin_coro_size? The argument will always be 1, right?
> > > > > It only starts to change in LLVM intrinsics, if I read the impl correctly.
> > > > Yeah, It is always 1 for Clang until the spec is fixed (then we could revert it back to 0).  Other clients using `__builtin_coro_size` may use 0 if the client doesn't care about overaligned frame or it could handle overaligned frame by itself. 
> > > BTW, is it OK to edit the `builtin`s directly? Since builtin is different with intrinsic which is only visible in the internal of compiler, builtin could be used by any end users. Although I know there should be  little users who would use `__builtin_coro` APIs, I worry if there is any guide principle for editing the `builtin`s.
> > > BTW, is it OK to edit the builtins directly? Since builtin is different with intrinsic which is only visible in the internal of compiler, builtin could be used by any end users. Although I know there should be little users who would use __builtin_coro APIs, I worry if there is any guide principle for editing the builtins.
> > 
> > I think it is ok to change these if it is justified like anything else.
> > 
> > builtins/intrinsics are interfaces on different levels. I'm trying to make __builtin_coro_size consistent with llvm.coro.size because I don't have a good reason for not doing that. (assume that we keep this opt-in overaligned frame handling in LLVM even after the spec is fixed since it helps solve a practical problem and the maintenance cost is low)
> > 
> > 
> It doesn't make sense to me that we need to change the signature for `__builtin_coro_size` in this patch. In other words, why do we need to change `__builtin_coro_size `? What are problems that can't be solved if we don't change `__builtin_coro_size`? At least, if it is necessary to change `__builtin_coro_size`, we could make it in successive patches.
Yeah I agree with ChuanqiXu, there is no need to make the builtin to be exactly the same as the llvm intrinsics just because they have the same name. Many of them are different even though they have the same name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100739