On January 20, 2017 3:56:31 PM GMT+01:00, David Malcolm <dmalc...@redhat.com>
wrote:
>On Fri, 2017-01-20 at 09:06 +0100, Richard Biener wrote:
>> On Thu, Jan 19, 2017 at 6:22 PM, David Malcolm <dmalc...@redhat.com>
>> wrote:
>> > On Mon, 2017-01-16 at 14:42 -0700, Jeff Law wrote:
>> > > On 01/09/2017 07:38 PM, David Malcolm wrote:
>> > > > gcc/ChangeLog:
>> > > > * passes.c: Include "insn-addr.h".
>> > > > (should_skip_pass_p): Add logging. Update logic for
>> > > > running
>> > > > "expand" to be compatible with both __GIMPLE and __RTL.
>> > > > Guard
>> > > > property-provider override so it is only done for gimple
>> > > > passes.
>> > > > Don't skip dfinit.
>> > > > (skip_pass): New function.
>> > > > (execute_one_pass): Call skip_pass when skipping passes.
>> > > > ---
>> > > > gcc/passes.c | 65
>> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
>> > > > 1 file changed, 58 insertions(+), 7 deletions(-)
>> > > >
>> > > > diff --git a/gcc/passes.c b/gcc/passes.c
>> > > > index 31262ed..6954d1e 100644
>> > > > --- a/gcc/passes.c
>> > > > +++ b/gcc/passes.c
>> > > > @@ -59,6 +59,7 @@ along with GCC; see the file COPYING3. If
>> > > > not
>> > > > see
>> > > > #include "cfgrtl.h"
>> > > > #include "tree-ssa-live.h" /* For remove_unused_locals. */
>> > > > #include "tree-cfgcleanup.h"
>> > > > +#include "insn-addr.h" /* for INSN_ADDRESSES_ALLOC. */
>> > > insn-addr? Yuk.
>> > >
>> > >
>> > > >
>> > > > using namespace gcc;
>> > > >
>> > > > @@ -2315,26 +2316,73 @@ should_skip_pass_p (opt_pass *pass)
>> > > > if (!cfun->pass_startwith)
>> > > > return false;
>> > > >
>> > > > - /* We can't skip the lowering phase yet -- ideally we'd
>> > > > - drive that phase fully via properties. */
>> > > > - if (!(cfun->curr_properties & PROP_ssa))
>> > > > - return false;
>> > > > + /* For __GIMPLE functions, we have to at least start when we
>> > > > leave
>> > > > + SSA. */
>> > > > + if (pass->properties_destroyed & PROP_ssa)
>> > > > + {
>> > > > + if (!quiet_flag)
>> > > > + fprintf (stderr, "starting anyway when leaving SSA: %s\n",
>> > > > pass->name);
>> > > > + cfun->pass_startwith = NULL;
>> > > > + return false;
>> > > > + }
>> > > This seems to need a comment -- it's not obvious how destroying
>> > > the
>> > > SSA
>> > > property maps to a pass that can not be skipped.
>> >
>> > Added:
>> >
>> > /* For __GIMPLE functions, we have to at least start when we
>> > leave
>> > SSA. Hence, we need to detect the "expand" pass, and stop
>> > skipping
>> > when we encounter it. A cheap way to identify "expand" is it
>> > to
>> > detect the destruction of PROP_ssa.
>> > For __RTL functions, we invoke "rest_of_compilation" directly,
>> > which
>> > is after "expand", and hence we don't reach this conditional.
>> > */
>> >
>> > > > - /* And also run any property provider. */
>> > > > - if (pass->properties_provided != 0)
>> > > > + /* Run any property provider. */
>> > > > + if (pass->type == GIMPLE_PASS
>> > > > + && pass->properties_provided != 0)
>> > > > return false;
>> > > So comment needed here too. I read this as "if a gimple pass
>> > > provides a
>> > > property then it should not be skipped. Which means that an RTL
>> > > pass
>> > > that provides a property can?
>> >
>> > Added:
>> >
>> > /* For GIMPLE passes, run any property provider (but continue
>> > skipping
>> > afterwards).
>> > We don't want to force running RTL passes that are property
>> > providers:
>> > "expand" is covered above, and the only pass other than
>> > "expand" that
>> > provides a property is "into_cfglayout" (PROP_cfglayout),
>> > which does
>> > too much for a dumped __RTL function. */
>> >
>> > ...the problem being that into_cfglayout's execute vfunc calls
>> > cfg_layout_initialize, which does a lot more that just
>> > cfg_layout_rtl_register_cfg_hooks (the skip hack does just the
>> > latter).
>> >
>> > > > + /* Don't skip df init; later RTL passes need it. */
>> > > > + if (strstr (pass->name, "dfinit") != NULL)
>> > > > + return false;
>> > > Which seems like a failing in RTL passes saying they need DF
>> > > init.
>> >
>> > There isn't a "PROP_df"; should there be?
>> > Or is this hack accepable?
>> >
>> > > > +/* Skip the given pass, for handling passes before "startwith"
>> > > > + in __GIMPLE and__RTL-marked functions.
>> > > > + In theory, this ought to be a no-op, but some of the RTL
>> > > > passes
>> > > > + need additional processing here. */
>> > > > +
>> > > > +static void
>> > > > +skip_pass (opt_pass *pass)
>> > > ...
>> > > This all feels like a failing in how we handle state in the RTL
>> > > world.
>> > > And I suspect it's prone to error. Imagine if I'm hacking on
>> > > something
>> > > in the RTL world and my code depends on something else being set
>> > > up.
>> > > I
>> > > really ought to have a way within my pass to indicate what I
>> > > depend
>> > > on.
>> > > Having it hidden away in passes.c makes it easy to miss/forget.
>> >
>> > Indeed, it's a hack. I preferred the vfunc idea, but Richi prefers
>> > to keep it all in one place.
>> >
>> > > > +{
>> > > > + /* Pass "reload" sets the global "reload_completed", and
>> > > > many
>> > > > + things depend on this (e.g. instructions in .md files).
>> > > > */
>> > > > + if (strcmp (pass->name, "reload") == 0)
>> > > > + reload_completed = 1;
>> > > Seems like this ought to be a property provided by LRA/reload.
>> >
>> > If we have a __RTL function with a "startwith" of a pass after
>> > reload,
>> > we don't want to run "reload" when iterating through the pass list
>> > to
>> > reach the start pass, since presumably it could change the insns.
>> > So
>> > if LRA/reload provide a property, say PROP_reload_completed, we'd
>> > still
>> > need a way to *not* run reload, whilst setting the reload_completed
>> > global. So I don't think that a property necessarily buys us much
>> > here (it'd still be a hack either way...).
>> >
>> > Or is your observation more about having a way to identify the pass
>> > without doing a strcmp?
>> >
>> > > > +
>> > > > + /* The INSN_ADDRESSES vec is normally set up by
>> > > > + shorten_branches; set it up for the benefit of passes
>> > > > that
>> > > > + run after this. */
>> > > > + if (strcmp (pass->name, "shorten") == 0)
>> > > > + INSN_ADDRESSES_ALLOC (get_max_uid ());
>> > > Similarly ought to be provided by shorten-branches
>> >
>> > Similar to the reload_completed discussion above.
>> >
>> >
>> > > > +
>> > > > + /* 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;
>> > > > + }
>> > > > +}
>> > > This feels somewhat different, but still a hack.
>> > >
>> > > I don't have strong suggestions on how to approach this, but what
>> > > we've
>> > > got here feels like a hack and one prone to bitrot.
>> >
>> > Given that Richi seems to prefer the "contain it all in once place"
>> > to the virtual function idea, there's not much more I can offer to
>> > fix it.
>> >
>> > Updated version of the patch attached (just adding the missing
>> > comments)
>> >
>> > Is this version OK?
>>
>> Ok.
>>
>> Richard.
>
>[...snip...]
>
>Thanks.
>
>Current status of the RTL "frontend" is that patches 1-8 are in trunk,
>so that we're building the code for reading RTL dumps, but patch 9 (the
>code to wire that up to cc1 with "__RTL" so that it's actually usable
>from DejaGnu) isn't.
>
>I believe the only remaining part of patch 9 that hasn't been approved
>yet is:
>
>"[PATCH 9f] Add a way for the C frontend to compile __RTL-tagged
>functions"
> https://gcc.gnu.org/ml/gcc-patches/2017-01/msg00590.html
>
>Richi: if that patch is approved, are you OK with patch 9 in early
>stage 4?
Yes.
