Re: [PATCH 3/N] Come up with casm global state.
On Fri, Nov 5, 2021 at 3:27 PM Martin Liška wrote:
>
> On 10/26/21 09:45, Richard Biener wrote:
> > On Mon, Oct 25, 2021 at 6:32 PM Segher Boessenkool
> > wrote:
> >>
> >> Hi!
> >>
> >> On Mon, Oct 25, 2021 at 03:36:25PM +0200, Martin Liška wrote:
> >>> --- a/gcc/config/rs6000/rs6000-internal.h
> >>> +++ b/gcc/config/rs6000/rs6000-internal.h
> >>> @@ -189,4 +189,13 @@ extern bool rs6000_passes_vector;
> >>> extern bool rs6000_returns_struct;
> >>> extern bool cpu_builtin_p;
> >>>
> >>> +struct rs6000_asm_out_state : public asm_out_state
> >>> +{
> >>> + /* Initialize ELF sections. */
> >>> + void init_elf_sections ();
> >>> +
> >>> + /* Initialize XCOFF sections. */
> >>> + void init_xcoff_sections ();
> >>> +};
> >>
> >> Our coding convention says to use "class", not "struct" (since this
> >> isn't valid C code at all).
> >>
> >>> - sdata2_section
> >>> + sec.sdata2
> >>> = get_unnamed_section (SECTION_WRITE, output_section_asm_op,
> >>> SDATA2_SECTION_ASM_OP);
> >>
> >> (broken indentation)
> >>
> >>> +/* Implement TARGET_ASM_INIT_SECTIONS. */
> >>
> >> That comment is out-of-date.
> >>
> >>> +static asm_out_state *
> >>> +rs6000_elf_asm_init_sections (void)
> >>> +{
> >>> + rs6000_asm_out_state *target_state
> >>> += new (ggc_alloc ()) rs6000_asm_out_state ();
> >>
> >> Hrm, maybe we can have a macro or function that does this, ggc_new or
> >> something?
> >>
> >>> +/* Implement TARGET_ASM_INIT_SECTIONS. */
> >>> +
> >>> +static asm_out_state *
> >>> +rs6000_xcoff_asm_init_sections (void)
> >>
> >> Here, too. Both implementations are each one of several functions that
> >> together implement the target macro.
> >>
> >>> +/* The section that holds the DWARF2 frame unwind information, when
> >>> known.
> >>> + The section is set either by the target's init_sections hook or
> >>> by the
> >>> + first call to switch_to_eh_frame_section. */
> >>> +section *eh_frame;
> >>> +
> >>> +/* RS6000 sections. */
> >>
> >> Nothing here? Just remove the comment header?
> >>
> >> The idea looks fine to me.
> >
> > Yeah, of course then the target hook does not need to do the allocation
> > and we could simply keep the current init_sections hook but change it
> > to take the asm_out_state to initialize as argument.
>
> Makes sense.
>
> >
> > Note that I'd put
> >
> > +/* RS6000 sections. */
> > +
> > +/* ELF sections. */
> > +section *toc;
> > +section *sdata2;
> > +
> > +/* XCOFF sections. */
> > +section *read_only_data;
> > +section *private_data;
> > +section *tls_data;
> > +section *tls_private_data;
> > +section *read_only_private_data;
> >
> > into a union, thus
> >
> > union {
> > struct /* RS6000 sections */ {
> > /* ELF sections. */
> >section *toc;
> > ...
> > } rs6000;
> > struct /* darwin sections */ {
> > ...
> > };
>
> Union is a bit tricky for GGC marking script, but we can manage that.
>
> >
> > not sure whether we need some magic GTY marking here to make
> > it pick up the 'correct' set. Another alternative would be
> >
> > section *target[MAX_TARGET_SECTIONS];
> >
> > and #defines in the targets mapping the former global variables to
> > indices in that array.
> >
> > All of this isn't "nice C++" of course, but well ... I'm not the one
> > to insist ;)
>
> Anyway, I took a look at targets that do call the init_sections hook and I
> noticed
> Darwin uses pretty many sections and comes up with an array that is defined
> here:
> ./gcc/config/darwin-sections.def
>
> I tend to creating all sections in asm_out_state with DEF_SECTION, where the
> list
> will be extensible with a target-specific definition list.
>
> What do you think Richi?
That sounds good to me as well.
Richard.
>
> Thanks,
> Martin
>
> >
> > Richard.
> >
> >>
> >> Segher
>
Re: [PATCH 3/N] Come up with casm global state.
On 10/26/21 09:45, Richard Biener wrote:
On Mon, Oct 25, 2021 at 6:32 PM Segher Boessenkool
wrote:
Hi!
On Mon, Oct 25, 2021 at 03:36:25PM +0200, Martin Liška wrote:
--- a/gcc/config/rs6000/rs6000-internal.h
+++ b/gcc/config/rs6000/rs6000-internal.h
@@ -189,4 +189,13 @@ extern bool rs6000_passes_vector;
extern bool rs6000_returns_struct;
extern bool cpu_builtin_p;
+struct rs6000_asm_out_state : public asm_out_state
+{
+ /* Initialize ELF sections. */
+ void init_elf_sections ();
+
+ /* Initialize XCOFF sections. */
+ void init_xcoff_sections ();
+};
Our coding convention says to use "class", not "struct" (since this
isn't valid C code at all).
- sdata2_section
+ sec.sdata2
= get_unnamed_section (SECTION_WRITE, output_section_asm_op,
SDATA2_SECTION_ASM_OP);
(broken indentation)
+/* Implement TARGET_ASM_INIT_SECTIONS. */
That comment is out-of-date.
+static asm_out_state *
+rs6000_elf_asm_init_sections (void)
+{
+ rs6000_asm_out_state *target_state
+= new (ggc_alloc ()) rs6000_asm_out_state ();
Hrm, maybe we can have a macro or function that does this, ggc_new or
something?
+/* Implement TARGET_ASM_INIT_SECTIONS. */
+
+static asm_out_state *
+rs6000_xcoff_asm_init_sections (void)
Here, too. Both implementations are each one of several functions that
together implement the target macro.
+/* The section that holds the DWARF2 frame unwind information, when known.
+ The section is set either by the target's init_sections hook or by the
+ first call to switch_to_eh_frame_section. */
+section *eh_frame;
+
+/* RS6000 sections. */
Nothing here? Just remove the comment header?
The idea looks fine to me.
Yeah, of course then the target hook does not need to do the allocation
and we could simply keep the current init_sections hook but change it
to take the asm_out_state to initialize as argument.
Makes sense.
Note that I'd put
+/* RS6000 sections. */
+
+/* ELF sections. */
+section *toc;
+section *sdata2;
+
+/* XCOFF sections. */
+section *read_only_data;
+section *private_data;
+section *tls_data;
+section *tls_private_data;
+section *read_only_private_data;
into a union, thus
union {
struct /* RS6000 sections */ {
/* ELF sections. */
section *toc;
...
} rs6000;
struct /* darwin sections */ {
...
};
Union is a bit tricky for GGC marking script, but we can manage that.
not sure whether we need some magic GTY marking here to make
it pick up the 'correct' set. Another alternative would be
section *target[MAX_TARGET_SECTIONS];
and #defines in the targets mapping the former global variables to
indices in that array.
All of this isn't "nice C++" of course, but well ... I'm not the one
to insist ;)
Anyway, I took a look at targets that do call the init_sections hook and I
noticed
Darwin uses pretty many sections and comes up with an array that is defined
here:
./gcc/config/darwin-sections.def
I tend to creating all sections in asm_out_state with DEF_SECTION, where the
list
will be extensible with a target-specific definition list.
What do you think Richi?
Thanks,
Martin
Richard.
Segher
Re: [PATCH 3/N] Come up with casm global state.
On Mon, Oct 25, 2021 at 6:32 PM Segher Boessenkool
wrote:
>
> Hi!
>
> On Mon, Oct 25, 2021 at 03:36:25PM +0200, Martin Liška wrote:
> > --- a/gcc/config/rs6000/rs6000-internal.h
> > +++ b/gcc/config/rs6000/rs6000-internal.h
> > @@ -189,4 +189,13 @@ extern bool rs6000_passes_vector;
> > extern bool rs6000_returns_struct;
> > extern bool cpu_builtin_p;
> >
> > +struct rs6000_asm_out_state : public asm_out_state
> > +{
> > + /* Initialize ELF sections. */
> > + void init_elf_sections ();
> > +
> > + /* Initialize XCOFF sections. */
> > + void init_xcoff_sections ();
> > +};
>
> Our coding convention says to use "class", not "struct" (since this
> isn't valid C code at all).
>
> > - sdata2_section
> > + sec.sdata2
> > = get_unnamed_section (SECTION_WRITE, output_section_asm_op,
> > SDATA2_SECTION_ASM_OP);
>
> (broken indentation)
>
> > +/* Implement TARGET_ASM_INIT_SECTIONS. */
>
> That comment is out-of-date.
>
> > +static asm_out_state *
> > +rs6000_elf_asm_init_sections (void)
> > +{
> > + rs6000_asm_out_state *target_state
> > += new (ggc_alloc ()) rs6000_asm_out_state ();
>
> Hrm, maybe we can have a macro or function that does this, ggc_new or
> something?
>
> > +/* Implement TARGET_ASM_INIT_SECTIONS. */
> > +
> > +static asm_out_state *
> > +rs6000_xcoff_asm_init_sections (void)
>
> Here, too. Both implementations are each one of several functions that
> together implement the target macro.
>
> > +/* The section that holds the DWARF2 frame unwind information, when
> > known.
> > + The section is set either by the target's init_sections hook or by
> > the
> > + first call to switch_to_eh_frame_section. */
> > +section *eh_frame;
> > +
> > +/* RS6000 sections. */
>
> Nothing here? Just remove the comment header?
>
> The idea looks fine to me.
Yeah, of course then the target hook does not need to do the allocation
and we could simply keep the current init_sections hook but change it
to take the asm_out_state to initialize as argument.
Note that I'd put
+/* RS6000 sections. */
+
+/* ELF sections. */
+section *toc;
+section *sdata2;
+
+/* XCOFF sections. */
+section *read_only_data;
+section *private_data;
+section *tls_data;
+section *tls_private_data;
+section *read_only_private_data;
into a union, thus
union {
struct /* RS6000 sections */ {
/* ELF sections. */
section *toc;
...
} rs6000;
struct /* darwin sections */ {
...
};
not sure whether we need some magic GTY marking here to make
it pick up the 'correct' set. Another alternative would be
section *target[MAX_TARGET_SECTIONS];
and #defines in the targets mapping the former global variables to
indices in that array.
All of this isn't "nice C++" of course, but well ... I'm not the one
to insist ;)
Richard.
>
> Segher
Re: [PATCH 3/N] Come up with casm global state.
Hi!
On Mon, Oct 25, 2021 at 03:36:25PM +0200, Martin Liška wrote:
> --- a/gcc/config/rs6000/rs6000-internal.h
> +++ b/gcc/config/rs6000/rs6000-internal.h
> @@ -189,4 +189,13 @@ extern bool rs6000_passes_vector;
> extern bool rs6000_returns_struct;
> extern bool cpu_builtin_p;
>
> +struct rs6000_asm_out_state : public asm_out_state
> +{
> + /* Initialize ELF sections. */
> + void init_elf_sections ();
> +
> + /* Initialize XCOFF sections. */
> + void init_xcoff_sections ();
> +};
Our coding convention says to use "class", not "struct" (since this
isn't valid C code at all).
> - sdata2_section
> + sec.sdata2
> = get_unnamed_section (SECTION_WRITE, output_section_asm_op,
> SDATA2_SECTION_ASM_OP);
(broken indentation)
> +/* Implement TARGET_ASM_INIT_SECTIONS. */
That comment is out-of-date.
> +static asm_out_state *
> +rs6000_elf_asm_init_sections (void)
> +{
> + rs6000_asm_out_state *target_state
> += new (ggc_alloc ()) rs6000_asm_out_state ();
Hrm, maybe we can have a macro or function that does this, ggc_new or
something?
> +/* Implement TARGET_ASM_INIT_SECTIONS. */
> +
> +static asm_out_state *
> +rs6000_xcoff_asm_init_sections (void)
Here, too. Both implementations are each one of several functions that
together implement the target macro.
> +/* The section that holds the DWARF2 frame unwind information, when
> known.
> + The section is set either by the target's init_sections hook or by the
> + first call to switch_to_eh_frame_section. */
> +section *eh_frame;
> +
> +/* RS6000 sections. */
Nothing here? Just remove the comment header?
The idea looks fine to me.
Segher
Re: [PATCH 3/N] Come up with casm global state.
On Mon, Oct 25, 2021 at 12:46:30PM +0200, Richard Biener wrote: > On Thu, Oct 21, 2021 at 5:42 PM Segher Boessenkool > wrote: > > It's disgusting, and fragile. The define is slightly better than having > > to write it out every time. But can this not be done properly? > > > > If you use object-oriented stuff and need casts for that, you are doing > > something wrong. > > I think the "proper" fix would be to make 'casm' have the correct type > in the first place +1 > - of course that would either mean that the target > needs to provide the (possibly derived) type, for example via a typedef > in the target structure or a classical target macro. If gengtype would > know about inheritance that would also fix the GC marking issue. Do we want gengtype to do inheritance, will it solve more future problems, or will it *cause* more problems later? > Of course encoding a static type in the target structure is the > wrong direction from making targets "switchable". Yes. > Other than that my C++ fu is too weak to suggest the correct "pattern" > here. Virtual functions something something? Segher
Re: [PATCH 3/N] Come up with casm global state.
On Thu, Oct 21, 2021 at 5:42 PM Segher Boessenkool
wrote:
>
> On Thu, Oct 21, 2021 at 02:42:10PM +0200, Richard Biener wrote:
> > +#define rs6000_casm static_cast (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).
>
> It's disgusting, and fragile. The define is slightly better than having
> to write it out every time. But can this not be done properly?
>
> If you use object-oriented stuff and need casts for that, you are doing
> something wrong.
I think the "proper" fix would be to make 'casm' have the correct type
in the first place - of course that would either mean that the target
needs to provide the (possibly derived) type, for example via a typedef
in the target structure or a classical target macro. If gengtype would
know about inheritance that would also fix the GC marking issue.
Of course encoding a static type in the target structure is the
wrong direction from making targets "switchable".
Other than that my C++ fu is too weak to suggest the correct "pattern"
here.
> > 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 ();
> > + 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).
>
> Yeah.
>
>
> Segher
Re: [PATCH 3/N] Come up with casm global state.
On Thu, Oct 21, 2021 at 02:42:10PM +0200, Richard Biener wrote:
> +#define rs6000_casm static_cast (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).
It's disgusting, and fragile. The define is slightly better than having
to write it out every time. But can this not be done properly?
If you use object-oriented stuff and need casts for that, you are doing
something wrong.
> 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 ();
> + 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).
Yeah.
Segher
Re: [PATCH 3/N] Come up with casm global state.
On Thu, Oct 21, 2021 at 3:43 PM Martin Liška wrote:
>
> On 10/21/21 14:42, Richard Biener wrote:
> > On Thu, Oct 21, 2021 at 11:47 AM Martin Liška wrote:
> >>
> >> On 10/5/21 13:54, Richard Biener wrote:
> >>> On Mon, Oct 4, 2021 at 1:13 PM Martin Liška wrote:
>
> On 9/22/21 11:59, Richard Biener wrote:
> > On Thu, Sep 16, 2021 at 3:12 PM Martin Liška 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::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
> >>> (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
Re: [PATCH 3/N] Come up with casm global state.
On 10/21/21 14:42, Richard Biener wrote:
On Thu, Oct 21, 2021 at 11:47 AM Martin Liška wrote:
On 10/5/21 13:54, Richard Biener wrote:
On Mon, Oct 4, 2021 at 1:13 PM Martin Liška wrote:
On 9/22/21 11:59, Richard Biener wrote:
On Thu, Sep 16, 2021 at 3:12 PM Martin Liška 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::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
(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.
+#define rs6000_casm static_cast (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 ();
+
Re: [PATCH 3/N] Come up with casm global state.
On Thu, Oct 21, 2021 at 2:48 PM David Malcolm via Gcc-patches wrote: > > On Thu, 2021-09-16 at 15:12 +0200, Martin Liška 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? > > Thanks, > > Martin > > This is possibly a silly question, but what does the "c" stand for in > "casm"? "compiler"? "compilation"? "common"? 'current' like cfun or crtl .. maybe just following "bad" examples? > Sorry if it's explained somewhere, but I didn't spot this when glancing > at the patches. > > Dave >
Re: [PATCH 3/N] Come up with casm global state.
On Thu, 2021-09-16 at 15:12 +0200, Martin Liška 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? > Thanks, > Martin This is possibly a silly question, but what does the "c" stand for in "casm"? "compiler"? "compilation"? "common"? Sorry if it's explained somewhere, but I didn't spot this when glancing at the patches. Dave
Re: [PATCH 3/N] Come up with casm global state.
On Thu, Oct 21, 2021 at 11:47 AM Martin Liška wrote:
>
> On 10/5/21 13:54, Richard Biener wrote:
> > On Mon, Oct 4, 2021 at 1:13 PM Martin Liška wrote:
> >>
> >> On 9/22/21 11:59, Richard Biener wrote:
> >>> On Thu, Sep 16, 2021 at 3:12 PM Martin Liška 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::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
> > (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 (casm)
maybe there's a better way? Though I
Re: [PATCH 3/N] Come up with casm global state.
On Mon, Oct 4, 2021 at 1:13 PM Martin Liška wrote:
>
> On 9/22/21 11:59, Richard Biener wrote:
> > On Thu, Sep 16, 2021 at 3:12 PM Martin Liška 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::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
(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.
Richard.
> Thanks,
> Cheers,
> Martin
>
>
> >
> > Richard.
> >
> >> Thanks,
> >> Martin
>
Re: [PATCH 3/N] Come up with casm global state.
On 9/16/21 15:12, Martin Liška 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? Thanks, Martin As explained in the '3/N' part, we will have to live with asm_out_file macro for some time. Is this renaming patch fine? Thanks, Martin
Re: [PATCH 3/N] Come up with casm global state.
On 9/22/21 11:59, Richard Biener wrote:
On Thu, Sep 16, 2021 at 3:12 PM Martin Liška 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::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?
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?
Thanks,
Cheers,
Martin
Richard.
Thanks,
Martin
Re: [PATCH 3/N] Come up with casm global state.
On Thu, Sep 16, 2021 at 3:12 PM Martin Liška 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? 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) You leave quite some global state out of the struct that is obviously related, in the diff I see object_block_htab for example. 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). 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. 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. 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? Richard. > Thanks, > Martin
