[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added inline comments.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4450
+    Value *Extra = Builder.CreateSelect(Cmp, Diff, Zero);
+    return RValue::get(Builder.CreateAdd(SizeCall, Extra));
   }
----------------
ychen wrote:
> rjmccall wrote:
> > Okay, so you're implicitly increasing the coroutine size to allow you to round up to get an aligned frame.  But how do you round back down to get the actual pointer that you need to delete?  This just doesn't work.
> > 
> > You really ought to just be using the aligned `operator new` instead when the required alignment is too high.  If that means checking the alignment "dynamically" before calling `operator new` / `operator delete`, so be it.  In practice, it will not be dynamic because lowering will replace the `coro.align` call with a constant, at which point the branch will be foldable.
> > 
> > I don't know what to suggest if the aligned `operator new` isn't reliably available on the target OS.  You could outline a function to pick the best allocator/deallocator, I suppose.
> Thanks for the review!
> 
> > Okay, so you're implicitly increasing the coroutine size to allow you to round up to get an aligned frame. But how do you round back down to get the actual pointer that you need to delete? This just doesn't work.
> 
> Hmm, you're right that I missed the `delete` part, thanks. The adjusted amount is constant, I could just dial it down in `coro.free`, right?
> 
> >  You really ought to just be using the aligned operator new instead when the required alignment is too high. If that means checking the alignment "dynamically" before calling operator new / operator delete, so be it. In practice, it will not be dynamic because lowering will replace the coro.align call with a constant, at which point the branch will be foldable.
>  
> That's my intuition at first. But spec is written in a way suggesting (IMHO) that the aligned version should not be used? What if the user specify their own allocator, then which one they should use?
Sorry, I meant the adjusted amount is coro.align - std::max_align_t,  I could subtract it in `coro.free` . I think it should work?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97915