Re: [PATCH 9e] Update "startwith" logic for pass-skipping to handle __RTL functions

[Jeff Law]

On 01/19/2017 02:26 AM, Richard Biener wrote:

On Wed, Jan 18, 2017 at 5:36 PM, Jeff Law  wrote:

On 01/17/2017 02:28 AM, Richard Biener wrote:



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.



All the above needs a bit of cleanup in the way we use (or not use)
PROP_xxx.
For example right now you can't startwith a __GIMPLE with a pass inside
the
loop pipeline because those passes expect loops to be initialized and be
in
loop-closed SSA.  And with the hack above for the property providers
you'll
always run pass_crited (that's a bad user of a PROP_).

Ideally we'd figure out required properties from the startwith pass
(but there's not
an easy way to compute it w/o actually "executing" the passes) and then
enable
enough passes on the way to it providing those properties.

Or finally restructure things in a way that the pass manager automatically
runs
property provider passes before passes requiring properties that are
not yet available...

Instead of those pass->name comparisions we could invent a new flag in the
pass structure whether a pass should always be run for __GIMPLE or ___RTL
but that's a bit noisy right now.

So I'm fine with the (localized) "hacks" for the moment.


David suggested that we could have a method in the pass manager that would
be run if the pass is skipped.  "run_if_skipped" or some such.

What I like about that idea is the hack and the real code end up in the same
place.  So someone working on (for example) reload has a much better chance
of catching that they need to update the run_if_skipped method as they make
changes to reload.  It doesn't fix all the problems in this space, but I
think it's cleaner than bundling the hacks into the pass manager itself.

Would that work for you?  It does for me.


I think that walks in the wrong direction and just distributes the
hack over multiple
files.

I'd rather have it in one place.
We disagree, but I don't feel strongly enough about it to object. 
Though I'll probably chime in regularly as the list of hacks grows or 
gets out-of-date  :-)



Jeff

Re: [PATCH 9e] Update "startwith" logic for pass-skipping to handle __RTL functions

[Richard Biener]

On Wed, Jan 18, 2017 at 5:36 PM, Jeff Law  wrote:
> On 01/17/2017 02:28 AM, Richard Biener wrote:
>>>
>>>
>>> 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.
>>
>>
>> All the above needs a bit of cleanup in the way we use (or not use)
>> PROP_xxx.
>> For example right now you can't startwith a __GIMPLE with a pass inside
>> the
>> loop pipeline because those passes expect loops to be initialized and be
>> in
>> loop-closed SSA.  And with the hack above for the property providers
>> you'll
>> always run pass_crited (that's a bad user of a PROP_).
>>
>> Ideally we'd figure out required properties from the startwith pass
>> (but there's not
>> an easy way to compute it w/o actually "executing" the passes) and then
>> enable
>> enough passes on the way to it providing those properties.
>>
>> Or finally restructure things in a way that the pass manager automatically
>> runs
>> property provider passes before passes requiring properties that are
>> not yet available...
>>
>> Instead of those pass->name comparisions we could invent a new flag in the
>> pass structure whether a pass should always be run for __GIMPLE or ___RTL
>> but that's a bit noisy right now.
>>
>> So I'm fine with the (localized) "hacks" for the moment.
>
> David suggested that we could have a method in the pass manager that would
> be run if the pass is skipped.  "run_if_skipped" or some such.
>
> What I like about that idea is the hack and the real code end up in the same
> place.  So someone working on (for example) reload has a much better chance
> of catching that they need to update the run_if_skipped method as they make
> changes to reload.  It doesn't fix all the problems in this space, but I
> think it's cleaner than bundling the hacks into the pass manager itself.
>
> Would that work for you?  It does for me.

I think that walks in the wrong direction and just distributes the
hack over multiple
files.

I'd rather have it in one place.

Richard.

> jeff
>

Re: [PATCH 9e] Update "startwith" logic for pass-skipping to handle __RTL functions

[David Malcolm]

On Wed, 2017-01-18 at 09:36 -0700, Jeff Law wrote:
> On 01/17/2017 02:28 AM, Richard Biener wrote:
> > > 
> > > 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.
> > 
> > All the above needs a bit of cleanup in the way we use (or not use)
> > PROP_xxx.
> > For example right now you can't startwith a __GIMPLE with a pass
> > inside the
> > loop pipeline because those passes expect loops to be initialized
> > and be in
> > loop-closed SSA.  And with the hack above for the property
> > providers you'll
> > always run pass_crited (that's a bad user of a PROP_).
> > 
> > Ideally we'd figure out required properties from the startwith pass
> > (but there's not
> > an easy way to compute it w/o actually "executing" the passes) and
> > then enable
> > enough passes on the way to it providing those properties.
> > 
> > Or finally restructure things in a way that the pass manager
> > automatically runs
> > property provider passes before passes requiring properties that
> > are
> > not yet available...
> > 
> > Instead of those pass->name comparisions we could invent a new flag
> > in the
> > pass structure whether a pass should always be run for __GIMPLE or
> > ___RTL
> > but that's a bit noisy right now.
> > 
> > So I'm fine with the (localized) "hacks" for the moment.
> David suggested that we could have a method in the pass manager that 
> would be run if the pass is skipped.  "run_if_skipped" or some such.
> 
> What I like about that idea is the hack and the real code end up in
> the 
> same place.  So someone working on (for example) reload has a much 
> better chance of catching that they need to update the run_if_skipped
> method as they make changes to reload.  It doesn't fix all the
> problems 
> in this space, but I think it's cleaner than bundling the hacks into
> the 
> pass manager itself.
> 
> Would that work for you?  It does for me.
> 
> jeff

FWIW I posted an implementation of the idea here:
  https://gcc.gnu.org/ml/gcc-patches/2017-01/msg01268.html

Re: [PATCH 9e] Update "startwith" logic for pass-skipping to handle __RTL functions

[Jeff Law]

On 01/17/2017 02:28 AM, Richard Biener wrote:


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.


All the above needs a bit of cleanup in the way we use (or not use) PROP_xxx.
For example right now you can't startwith a __GIMPLE with a pass inside the
loop pipeline because those passes expect loops to be initialized and be in
loop-closed SSA.  And with the hack above for the property providers you'll
always run pass_crited (that's a bad user of a PROP_).

Ideally we'd figure out required properties from the startwith pass
(but there's not
an easy way to compute it w/o actually "executing" the passes) and then enable
enough passes on the way to it providing those properties.

Or finally restructure things in a way that the pass manager automatically runs
property provider passes before passes requiring properties that are
not yet available...

Instead of those pass->name comparisions we could invent a new flag in the
pass structure whether a pass should always be run for __GIMPLE or ___RTL
but that's a bit noisy right now.

So I'm fine with the (localized) "hacks" for the moment.
David suggested that we could have a method in the pass manager that 
would be run if the pass is skipped.  "run_if_skipped" or some such.


What I like about that idea is the hack and the real code end up in the 
same place.  So someone working on (for example) reload has a much 
better chance of catching that they need to update the run_if_skipped 
method as they make changes to reload.  It doesn't fix all the problems 
in this space, but I think it's cleaner than bundling the hacks into the 
pass manager itself.


Would that work for you?  It does for me.

jeff

Re: [PATCH 9e] Update "startwith" logic for pass-skipping to handle __RTL functions

[Richard Biener]

On Mon, Jan 16, 2017 at 10:42 PM, 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.
>>
>>
>>
>> -  /* 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?
>
>
>>
>> +  /* 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.
>
>
>
>> +/* 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.
>
>
>> +{
>> +  /* 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.
>
>
>> +
>> +  /* 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
>
>> +
>> +  /* 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.

All the above needs a bit of cleanup in the way we use (or not use) PROP_xxx.
For example right now you can't startwith a __GIMPLE with a pass inside the
loop pipeline because those passes expect loops to be initialized and be in
loop-closed SSA.  And with the hack above for the property providers you'll
always run pass_crited (that's a bad user of a PROP_).

Ideally we'd figure out required properties from the startwith pass
(but there's not
an easy way to compute it w/o actually "executing" the passes) and then enable
enough passes on the way to it providing those properties.

Or finally restructure things in a way that the pass 

Re: [PATCH 9e] Update "startwith" logic for pass-skipping to handle __RTL functions

[Jeff Law]

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.



-  /* 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?





+  /* 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.




+/* 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.




+{
+  /* 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.



+
+  /* 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


+
+  /* 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.


jeff

[PATCH 9e] Update "startwith" logic for pass-skipping to handle __RTL functions

[David Malcolm]

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.  */
 
 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;
+}
 
   if (determine_pass_name_match (pass->name, cfun->pass_startwith))
 {
+  if (!quiet_flag)
+   fprintf (stderr, "found starting pass: %s\n", pass->name);
   cfun->pass_startwith = NULL;
   return false;
 }
 
-  /* 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;
 
+  /* Don't skip df init; later RTL passes need it.  */
+  if (strstr (pass->name, "dfinit") != NULL)
+return false;
+
+  if (!quiet_flag)
+fprintf (stderr, "skipping pass: %s\n", pass->name);
+
   /* If we get here, then we have a "startwith" that we haven't seen yet;
  skip the pass.  */
   return true;
 }
 
+/* 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)
+{
+  /* 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;
+
+  /* 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 ());
+
+  /* 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;
+}
+}
+
 /* Execute PASS. */
 
 bool
@@ -2375,7 +2423,10 @@ execute_one_pass (opt_pass *pass)
 }
 
   if (should_skip_pass_p (pass))
-return true;
+{
+  skip_pass (pass);
+  return true;
+}
 
   /* Pass execution event trigger: useful to identify passes being
  executed.  */
-- 
1.8.5.3