On Tue, Nov 15, 2016 at 10:07 PM, David Malcolm <dmalc...@redhat.com> wrote:
> On Mon, 2016-11-14 at 16:14 +0100, Richard Biener wrote:
>> On Fri, Nov 11, 2016 at 10:15 PM, David Malcolm <dmalc...@redhat.com>
>> wrote:
>> > Changed in this version:
>> >
>> > * Rather than running just one pass, run *all* passes, but start at
>> > the given pass; support for "dg-do run" tests that execute the
>> > resulting code.
>> > * Updated test cases to new "compact" dump format; more test cases;
>> > use "dg-do run" in various places.
>> > * Lots of bugfixing
>> >
>> > Links to previous versions:
>> > https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00263.html
>> > https://gcc.gnu.org/ml/gcc-patches/2016-10/msg00500.html
>
>> Does running the RTL passes right from the parser work with -fsyntax
>> -only?
>
> It depends what you mean by "work". If I run it with -fsyntax-only,
> then pass_rest_of_compilation::gate returns false, and none of the RTL
> passes are run. Is this behavior correct?
Yes.
>> Doing it like __GIMPLE has the advantage of not exposing
>> "rest_of_compilation", etc..
>
> The gimple part of the compiler supports having multiple functions in
> memory at once, and then compiling them in arbitrary order based on
> decisions made by the callgraph.
>
> By contrast, the RTL part of the compiler is full of singleton state:
> things like crtl (aka x_rtl), the state of emit-rtl.c,
> "reload_completed", etc etc.
Ah, of course - I forgot that...
> To try to limit the scope of the project, for the RTL frontend work I'm
> merely attempting to restore the singleton RTL state from a dump,
> rather than to support having per function stashes of RTL state.
>
> Hence the rest of compilation gets invoked directly from the frontend
> for the __RTL case, since it will get overwritten if there's a second
> __RTL function in the source file (which sounds like an idea for a test
> case; I'll attempt such a test case).
>
> I hope this is a reasonable approach. If not, I suppose I can attempt
> to bundle it up into some kind of RTL function state, but that seems
> like significant scope creep.
Yeah, I think it's a good approach for now.
>> I'm now handling __GIMPLE from within declspecs (the GIMPLE FE stuff
>> has been committed), it would be nice to match the __RTL piece here.
>
> (Congratulations on getting the GIMPLE FE stuff in)
>
> I'm not sure I understand you here - do you want me to rewrite the
> __RTL parsing to match the __GIMPLE piece, or the other way around?
> If the former, presumably I should reuse (and rename)
> c_parser_gimple_pass_list?
Handle __RTL like __GIMPLE, thus as declspec. Of course ultimatively
Joseph has the last word here.
>
>> > gcc/ChangeLog:
>> > * Makefile.in (OBJS): Add run-rtl-passes.o.
>> >
>> > gcc/c-family/ChangeLog:
>> > * c-common.c (c_common_reswords): Add "__RTL".
>> > * c-common.h (enum rid): Add RID_RTL.
>> >
>> > gcc/c/ChangeLog:
>> > * c-parser.c: Include "read-rtl-function.h" and
>> > "run-rtl-passes.h".
>> > (c_parser_declaration_or_fndef): In the "GNU extensions"
>> > part of
>> > the leading comment, add an alternate production for
>> > "function-definition", along with new "rtl-body-specifier"
>> > and
>> > "rtl-body-pass-specifier" productions. Handle "__RTL" by
>> > calling
>> > c_parser_parse_rtl_body. Convert a timevar_push/pop pair
>> > to an auto_timevar, to cope with early exit.
>> > (c_parser_parse_rtl_body): New function.
>> >
>> > gcc/ChangeLog:
>> > * cfg.c (free_original_copy_tables): Remove assertion
>> > on original_copy_bb_pool.
>>
>> How can that trigger?
>
> It happens when running pass_outof_cfg_layout_mode::execute; seen with
> gcc.dg/rtl/x86_64/test-return-const.c.before-fwprop.c.
>
> The input file is a dump taken in cfg_layout mode (although in this
> case it's a trivial 3-basic-block CFG, but ideally there would be cases
> with non-trivial control flow); it has "fwprop1" as its starting pass.
>
> Running without -quiet shows:
>
> skipping pass: *rest_of_compilation
> skipping pass: vregs
> skipping pass: into_cfglayout
> skipping pass: jump
> skipping pass: subreg1
> skipping pass: cse1
> found starting pass: fwprop1
>
> i.e. it skips the into_cfglayout (amongst others), to start with
> fwprop1.
>
> In theory skipping a pass ought to be a no-op, assuming that we're
> faithfully reconstructing all RTL state. However, RTL state management
> is fiddly, so the patch introduces some logic in passes.c to do some
> things when skipping a pass; in particular, it has:
>
> /* Update the cfg hooks as appropriate. */
> if (strcmp (pass->name, "into_cfglayout") == 0)
> {
> cfg_layout_rtl_register_cfg_hooks ();
> cfun->curr_properties |= PROP_cfglayout;
> }
> if (strcmp (pass->name, "outof_cfglayout") == 0)
> {
> rtl_register_cfg_hooks ();
> cfun->curr_properties &= ~PROP_cfglayout;
> }
>
> so that even when skipping "into_cfglayout", the CFG hooks are at least
> correct.
I suppose the pass manager could handle the hook switching based on
a (RTL) pass setting/clearing PROP_cfglayout. Or all passes needing
cfglayout would need to set prop_required accordingly and thus
the into/outouf cfglayout passes would be implicit.
> The assertion fires when running outof_cfglayout later on (rather than
> skipping it); the assertion:
>
> gcc_assert (original_copy_bb_pool);
>
> assumes that into_cfglayout was actually run, rather than just the
> simple "skipping" version of it.
Ah, I see cfg_layout_initialize calls initialize_original_copy_tables ()
for whatever reason and they are kept initialized until out-of-cfglayout...
GIMPLE passes do init/free them all the time. I think a better
fix would be to add a original_copy_tables_initialized () function
and guard the free_original_copy_tables call with that (keeping
the assertion that they run in pairs).
>> > * cgraph.h (symtab_node::native_rtl_p): New decl.
>> > * cgraphunit.c (symtab_node::native_rtl_p): New function.
>> > (symtab_node::needed_p): Don't assert for early assembly
>> > output
>> > for __RTL functions.
>> > (cgraph_node::finalize_function): Set "force_output" for
>> > __RTL
>> > functions.
>> > (cgraph_node::analyze): Bail out early for __RTL functions.
>> > (analyze_functions): Update assertion to support __RTL
>> > functions.
>> > (cgraph_node::expand): Bail out early for __RTL functions.
>> > * emit-rtl.c (unshare_all_rtl_again): Wrap set_used_decls
>> > call
>> > in check for DECL_INITIAL.
>>
>> You should simply set DECL_INITIAL of your function decl (make_node
>> (BLOCK);).
>> There's too much code assuming that is not NULL (and I've fixed quite
>> a bit of
>> code during stage1 not doing that).
>
> Thanks; fixed.
>
>> > * final.c (rest_of_clean_state): Don't call delete_tree_ssa
>> > for
>> > _RTL functions.
>> > * function.h (struct function): Add field "native_RTL".
>>
>> I wonder if you could simply use ->curr_properties & PROP_rtl? (and
>> set that
>> property during parsing, of course)
>
> I tried to doing that; it mostly works, but this assertion fails:
>
> 237 symtab_node::needed_p (void)
> 238 {
> 239 /* Double check that no one output the function into assembly file
> 240 early. */
> 241 if (!native_rtl_p ())
> 242 gcc_checking_assert
> 243 (!DECL_ASSEMBLER_NAME_SET_P (decl)
> 244 || !TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl)));
>
> [I added the "if (!native_rtl_p ())" guard in the patch]
>
> The issue here is that when this is run, the __RTL function has been
> compiled and cleaned up, and the curr_properties & PROP_rtl has been
> cleared:
>
> (gdb) p decl->function_decl->f->curr_properties
> $7 = 0
>
> I could set the flag again after running the passes; on doing so, the
> test suite successfully runs.
Hmm, so if you are finished with the function you should set
symtab_node->analyzed
to false I think (it signals it's now merely an "extern" symbol and no
longer has a body).
Or set body_removed. Maybe just ask Honza what to do ...
Richard.
> Would you prefer I use curr_properties & PROP_rtl for this?
> [...snip...]
>
> Thanks
> Dave
