On 01/19/2017 10:22 AM, David Malcolm wrote:
...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?
I think it's acceptable for now. I think long term we're going to want
some kind of DF properties.
+{
+ /* 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?
It's not really the strcmp, but the general hackishness of this stuff.
I would also have preferred the virtual function for skipping a pass,
but not enough to argue that much about it.
+
+ /* 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.
Right.
+
+ /* 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.
Agreed/
Updated version of the patch attached (just adding the missing comments)
Is this version OK?
Changed in v2:
- added some comments to should_skip_pass_p
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.
OK. THanks for the updates. It should make it easier for others to see
what's going on in here.
jeff
