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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
