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.
+#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.
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.
Cheers,
Martin
Thanks,
Richard.
Thanks,
Martin
Richard.
Thanks,
Cheers,
Martin
Richard.
Thanks,
Martin
