Rebased the patch to GCC 14 trunk. Bootstrapped and regression tested again on x86_64-pc-linux-gnu, only difference is the new test failing without the patch.
On Jul 13 2022, at 2:54 pm, Michal Jankovic <michal.jankovi...@gmail.com> wrote: > Hi Iain, > > thanks for the info. I have some follow-up questions. > > On Jul 12 2022, at 7:11 pm, Iain Sandoe <i...@sandoe.co.uk> wrote: > >> Hi Michal, >> >>> On 12 Jul 2022, at 16:14, Michal Jankovič >>> <michal.jankovi...@gmail.com> wrote: >> >>> One other related thing I would like to investigate is reducing the >>> number of compiler generated variables in the frame, particularly >>> _Coro_destroy_fn and _Coro_self_handle. >>> >>> As I understand it, _Coro_destroy_fn just sets a flag in >>> _Coro_resume_index and calls _Coro_resume_fn; it should be possible to >>> move this logic to __builtin_coro_destroy, so that only _Coro_resume_fn >>> is stored in the frame; >> >> That is a particular point about GCC’s implementation … (it is not >> neccesarily, or even >> likely to be the same for other implementations) - see below. >> >> I was intending to do experiment with making the ramp/resume/destroy >> value a parameter >> to the actor function so that we would have something like - >> >> ramp calls actor(frame, 0) >> resume calls actor(frame, 1) >> destroy calls actor(frame, 2) >> - the token values are illustrative, not intended to be a final version. >> >> I think that should allow for more inlining opportunites and possibly >> a way forward to >> frame elision (a.k.a halo). >> >>> this would however change the coroutine ABI - I don't know if that's >>> a problem. >> >> The external ABI for the coroutine is the >> resume, >> destroy pointers >> and the promise >> and that one can find each of these from the frame pointer. >> >> This was agreed between the interested “vendors” so that one compiler >> could invoke >> coroutines built by another. So I do not think this is so much a >> useful area to explore. >> > > I understand. I still want to try to implement a more light-weight frame > layout with just one function pointer; would it be possible to merge > such a change if it was made opt-in via a compiler flag, eg > `-fsmall-coroutine-frame`? My use-case for this is embedded environments > with very limited memory, and I do not care about interoperability with > other compilers there. > >> Also the intent is that an indirect call through the frame pointer is >> the most frequent >> operation so should be the most efficient. >> resume() might be called many times, >> destroy() just once thus it is a cold code path >> - space can be important too - but interoperability was the goal here. >> >>> The _Coro_self_handle should be constructible on-demand from the >>> frame address. >> >> Yes, and in the header the relevant items are all constexpr - so that >> should happen in the >> user’s code. I elected to have that value in the frame to avoid >> recreating it each time - I >> suppose that is a trade-off of one oiptimisation c.f. another … > > If the handle construction cannot be optimized out, and its thus > a tradeoff between frame size and number of instructions, then this > could also be enabled by a hypothetical `-fsmall-coroutine-frame`. > > Coming back to this: > >>>> (the other related optimisation is to eliminate frame entries for >>>> scopes without any suspend >>>> points - which has the potential to save even more space for code with >>>> sparse use of co_xxxx) > > This would be nice; although it could encompassed by a more general > optimization - eliminate frame entries for all variables which are not > > accessed (directly or via pointer / reference) beyond a suspend point. > To be fair, I do not know how to get started on such an optimization, > or if it is even possible to do on the frontend. This would however be > immensely useful for reducing the frame size taken-up by complicated > co_await expressions (among other things), for example, if I have a > composed operation: > > co_await when_either(get_leaf_awaitable_1(), get_leaf_awaitable_2()); > > Right now, this creates space in the frame for the temporary 'leaf' > awaitables, which were already moved into the composed awaitable. > If the awaitable has an operator co_await that returns the real awaiter, > the original awaitable is also stored in the frame, even if it > is not referenced by the awaiter; another unused object gets stored if > > the .await_transform() customization point was used. > > What are your thoughts on the feasibility / difficulty of implementing > such an optimization? > > Michal > >>> >>> Do you have any advice / opinions on this before I try to implement it? >> >> Hopefully, the notes above help. >> >> I will rebase my latest code changes as soon as I have a chance and >> put them somewhere >> for you to look at - basically, these are to try and address the >> correctness issues we face, >> >> Iain >> >> >>> >>> Michal >>> >>> On Jul 12 2022, at 4:08 pm, Iain Sandoe <i...@sandoe.co.uk> wrote: >>> >>>> Hi Michal, >>>> >>>>> On 12 Jul 2022, at 14:35, Michal Jankovič via Gcc-patches >>>>> <gcc-patches@gcc.gnu.org> wrote: >>>>> >>>>> Currently, coroutine frames store all variables of a coroutine separately, >>>>> even if their lifetime does not overlap (they are in distinct >>>>> scopes). This >>>>> patch implements overlapping distinct variable scopes in the >>>>> coroutine frame, >>>>> by storing the frame fields in nested unions of structs. This lowers >>>>> the size >>>>> of the frame for larger coroutines significantly, and makes them >>>>> more usable >>>>> on systems with limited memory. >>>> >>>> not a review (I will try to take a look at the weekend). >>>> >>>> but … this is one of the two main optimisations on my TODO - so cool >>>> for doing it. >>>> >>>> (the other related optimisation is to eliminate frame entries for >>>> scopes without any suspend >>>> points - which has the potential to save even more space for code with >>>> sparse use of co_xxxx) >>>> >>>> Iain >>>> >>>>> Bootstrapped and regression tested on x86_64-pc-linux-gnu; new >>>>> test fails >>>>> before the patch and succeeds after with no regressions. >>>>> >>>>> PR c++/105989 >>>>> >>>>> gcc/cp/ChangeLog: >>>>> >>>>> * coroutines.cc (struct local_var_info): Add field_access_path. >>>>> (build_local_var_frame_access_expr): New. >>>>> (transform_local_var_uses): Use build_local_var_frame_access_expr. >>>>> (coro_make_frame_entry_id): New. >>>>> (coro_make_frame_entry): Delegate to coro_make_frame_entry_id. >>>>> (struct local_vars_frame_data): Add orig, field_access_path. >>>>> (register_local_var_uses): Generate new frame layout. Create access >>>>> paths to vars. >>>>> (morph_fn_to_coro): Set new fields in local_vars_frame_data. >>>>> >>>>> gcc/testsuite/ChangeLog: >>>>> >>>>> * g++.dg/coroutines/pr105989.C: New test. >>>>> >>>>> <pr105989.patch> >>>> >>>> >> >> >
pr105989.patch
Description: Binary data
