On Thu, Oct 21, 2021 at 11:47 AM Martin Liška <mli...@suse.cz> wrote: > > On 10/5/21 13:54, Richard Biener wrote: > > On Mon, Oct 4, 2021 at 1:13 PM Martin Liška <mli...@suse.cz> wrote: > >> > >> On 9/22/21 11:59, Richard Biener wrote: > >>> On Thu, Sep 16, 2021 at 3:12 PM Martin Liška <mli...@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 > >>