Re: [PATCH 3/N] Come up with casm global state.

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Oct 21, 2021 at 11:47 AM Martin Liška <mliska@suse.cz> wrote:
>
> On 10/5/21 13:54, Richard Biener wrote:
> > On Mon, Oct 4, 2021 at 1:13 PM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> On 9/22/21 11:59, Richard Biener wrote:
> >>> On Thu, Sep 16, 2021 at 3:12 PM Martin Liška <mliska@suse.cz> wrote:
> >>>>
> >>>> This patch comes up with asm_out_state and a new global variable casm.
> >>>>
> >>>> Tested on all cross compilers.
> >>>> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> >>>>
> >>>> Ready to be installed?
> >>>
> >>>           * output.h (struct asm_out_file): New struct that replaces a
> >>> ^^^
> >>> asm_out_state?
> >>
> >> Yes, sure!
> >>
> >>>
> >>> You replace a lot of asm_out_file - do we still need the macro then?
> >>> (I'd have simplified the patch leaving that replacement out at first)
> >>
> >> Well, I actually replaced only a very small fraction of the usage of asm_out_file.
> >>
> >> $ git grep asm_out_file | grep -v ChangeLog | wc -l
> >>
> >> 1601
> >>
> >>
> >> So I think we should live with the macro for some time ...
> >>
> >>>
> >>> You leave quite some global state out of the struct that is obviously
> >>> related, in the diff I see object_block_htab for example.
> >>
> >> Yes, I'm aware of it. Can we do that incrementally?
> >>
> >>> Basically
> >>> everything initialized in init_varasm_once is a candidate (which
> >>> then shows const_desc_htab and shared_constant_pool as well
> >>> as pending_assemble_externals_set).
> >>
> >> Well, these would probably need a different header file (or another #include ... must
> >> be added before output.h :// ).
> >>
> >>> For the goal of outputting
> >>> early DWARF to another file the state CTOR could have a mode
> >>> to not initialize those parts or we could have asm-out-state-with-sections
> >>> as base of asm-out-state.
> >>
> >> Yes, right now asm_out_state ctor is minimal:
> >>
> >>     asm_out_state (): out_file (NULL), in_section (NULL),
> >>       sec ({}), anchor_labelno (0), in_cold_section_p (false)
> >>     {
> >>       section_htab = hash_table<section_hasher>::create_ggc (31);
> >>     }
> >>
> >>>
> >>> In the end there will be a target part of the state so I think
> >>> construction needs to be defered to the target which can
> >>> derive from asm-out-state and initialize the part it needs.
> >>> That's currently what targetm.asm_out.init_sections () does
> >>> and we'd transform that to a targetm.asm_out.create () or so.
> >>> That might already be necessary for the DWARF stuff.
> >>
> >> So what do you want to with content of init_varasm_once function?
> >
> > It goes away ;)  Or rather becomes the invocation of the
> > asm-out-state CTOR via the target hook.
> >
> >>>
> >>> That said, dealing with the target stuff piecemail is OK, but maybe
> >>> try to make sure that init_varasm_once is actually identical
> >>> to what the CTOR does?
> >>
> >> So you want asm_out_state::asm_out_state doing what we current initialize
> >> in init_varasm_once, right?
> >
> > Yes, asm_out_state should cover everything initialized in init_varasm_once
> > (and the called targetm.asm_out.init_sections).
> >
> > targetm.asm_out.init_sections would become
> >
> > asm_out_state *init_sections ();
> >
> > where the target can derive from asm_out_state (optionally) and
> > allocates the asm-out-state & invokes the CTORs.  The middle-end
> > visible asm_out_state would contain all the tables initialized in
> > init_varasm_once.
> >
> > I'm not sure if doing it piecemail is good - we don't want to touch
> > things multiple times without good need and it might be things
> > turn out not be as "simple" as the above plan sounds.
> >
> > I would suggest to try the conversion with powerpc since it
> > seems to be the only target with two different implementations
> > for the target hook.
> >
> > The
> >
> > static GTY(()) section *read_only_data_section;
> > static GTY(()) section *private_data_section;
> > static GTY(()) section *tls_data_section;
> > static GTY(()) section *tls_private_data_section;
> > static GTY(()) section *read_only_private_data_section;
> > static GTY(()) section *sdata2_section;
> >
> > section *toc_section = 0;
> >
> > could become #defines to static_cast<rs6000_asm_out_state *>
> > (casm)->section_name
> > and there'd be
> >
> > class rs6000_asm_out_state : public asm_out_state
> > {
> > ... add stuff ...
> > }
> >
> > in rs6000/xcoff.h and rs6000/sysv4.h and some generic rs6000 header as well
> > (adding no fields) and the target hook would basically do
> >
> >   return ggc_new rs6000_asm_state ();
> >
> > (OK, ggc_alloc + placement new I guess).  The default hook just
> > creates asm_out_state.
>
> Hello.
>
> All right, I made a patch candidate that does the suggested steps and I ported
> rs6000 target (both ELF and XCOFF sub-targets).
>
> So far I was able to bootstrap on ppc6le-linux-gnu.
>
> Thoughts?

+extern GTY(()) rs6000_asm_out_state *target_casm;

this seems to be unused

+#define rs6000_casm static_cast<rs6000_asm_out_state *> (casm)

maybe there's a better way?  Though I can't think of one at the moment.
There are only 10 uses so eventually we can put the
static_cast into all places.  Let's ask the powerpc maintainers (CCed).

Note you do

+/* Implement TARGET_ASM_INIT_SECTIONS.  */
+
+static asm_out_state *
+rs6000_elf_asm_init_sections (void)
+{
+  rs6000_asm_out_state *target_state
+    = new (ggc_alloc<rs6000_asm_out_state> ()) rs6000_asm_out_state ();
+  target_state->init_elf_sections ();
+  target_state->init_sections ();
+
+  return target_state;
+}

If you'd have made init_sections virtual the flow would be more
natural and we could separate section init from casm construction
(and rs6000 would override init_sections but call the base function
from the override).

Alternatively asm_out_state::init_sections could move into the
CTOR itself so the target hook doesn't need to explicitely call it
(I guess I prefer that variant).

Otherwise looks great - there are of course a few other targets
left to fixup.

Thanks,
Richard.

> Thanks,
> Martin
>
> >
> > Richard.
> >
> >> Thanks,
> >> Cheers,
> >> Martin
> >>
> >>
> >>>
> >>> Richard.
> >>>
> >>>> Thanks,
> >>>> Martin
> >>