On Thu, Oct 21, 2021 at 3:43 PM Martin Liška <mli...@suse.cz> wrote:
>
> On 10/21/21 14:42, Richard Biener wrote:
> > 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
>
> Sure, it can be a static variable in rs6000.c for GGC marking purpose.
Ah, it's for GGC marking. In that case we should probably use
user marking to be able to use the casm root and call a virtual
mark() function from the ggc_mark overload? I failed to see the place
you initialize the global variable btw.
> >
> > +#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).
>
> Or we can wrap it with a function call doing the casting?
>
> >
> > 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).
>
> Yes, we can do an overridable function. Note there's small trick in the base
> init_sections function:
>
> if (readonly_data_section == NULL)
> readonly_data_section = text_section;
>
> So it needs to be run after a derived version.
Uh. Maybe we can unconditionally set it to text_section and
simply have the target override it if needed.
> >
> > Otherwise looks great - there are of course a few other targets
> > left to fixup.
>
> Yep. To be honest I'm quite happy with the current implementation and let's
> see what PPC guys say about it.
Yeah. Let's think of the GGC stuff a bit, having extra globals for the GC roots
of the correct type doesn't look sustainable? That is, I'd have
+struct GTY((user)) asm_out_state
+{...
virtual void gt_ggc_mx ();
};
void
asm_out_state::gt_ggc_mx ()
{
ggc_test_and_set_mark (sec.text);
...
}
void
gt_ggc_mx (asm_out_state *s)
{
s->gt_ggc_mx ();
}
and have targets derive gt_ggc_mx for their members (well, ideally of course
gengtype would magically provide those definitions...). The above also means
we could allocate asm_out_state on the heap(?)
> Cheers,
> Martin
>
> >
> > Thanks,
> > Richard.
> >
> >> Thanks,
> >> Martin
> >>
> >>>
> >>> Richard.
> >>>
> >>>> Thanks,
> >>>> Cheers,
> >>>> Martin
> >>>>
> >>>>
> >>>>>
> >>>>> Richard.
> >>>>>
> >>>>>> Thanks,
> >>>>>> Martin
> >>>>
>
