* [PATCH] make has_gate and has_execute useless
@ 2013-11-07 16:40 tsaunders
2013-11-07 17:51 ` Jeff Law
2013-11-08 9:44 ` Richard Biener
0 siblings, 2 replies; 12+ messages in thread
From: tsaunders @ 2013-11-07 16:40 UTC (permalink / raw)
To: gcc-patches; +Cc: Trevor Saunders
From: Trevor Saunders <tsaunders@mozilla.com>
Hi,
This is the result of seeing what it would take to get rid of the has_gate and
has_execute flags on pass_data. It turns out not much, but I wanted
confirmation this part is ok before I go deal with all the places that
initialize the fields.
I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite
regressions (ignoring the silk stuff because the full paths in its test names
break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok?
Trev
2013-11-06 Trevor Saunders <tsaunders@mozilla.com>
* pass_manager.h (pass_manager): Adjust.
* passes.c (opt_pass::execute): Tell the pass manager it doesn't need
to do anything for this pass.
(pass_manager::register_dump_files_1): Don't uselessly deal with
properties of passes.
(pass_manager::register_dump_files): Adjust.
(dump_one_pass): Just call pass->gate ().
(execute_ipa_summary_passes): Likewise.
(execute_one_pass): Don't check pass->has_execute flag.
(ipa_write_summaries_2): Don't check pass->has_gate flag.
(ipa_write_optimization_summaries_1): Likewise.
(ipa_read_summaries_1): Likewise.
(ipa_read_optimization_summaries_1): Likewise.
(execute_ipa_stmt_fixups): Likewise.
* tree-pass.h (pass_data): Rename has_gate to useless_has_gate and
has_execute to useless_has_execute to be sure they're unused.
(TODO_absolutely_nothing): New constant.
diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
index 77d78eb..3bc0a99 100644
--- a/gcc/pass_manager.h
+++ b/gcc/pass_manager.h
@@ -93,7 +93,7 @@ public:
private:
void set_pass_for_id (int id, opt_pass *pass);
- int register_dump_files_1 (struct opt_pass *pass, int properties);
+ void register_dump_files_1 (struct opt_pass *pass);
void register_dump_files (struct opt_pass *pass, int properties);
private:
diff --git a/gcc/passes.c b/gcc/passes.c
index 19e5869..3b28dc9 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -112,7 +112,7 @@ opt_pass::gate ()
unsigned int
opt_pass::execute ()
{
- return 0;
+ return TODO_absolutely_nothing;
}
opt_pass::opt_pass (const pass_data &data, context *ctxt)
@@ -701,33 +701,21 @@ pass_manager::register_one_dump_file (struct opt_pass *pass)
/* Recursive worker function for register_dump_files. */
-int
+void
pass_manager::
-register_dump_files_1 (struct opt_pass *pass, int properties)
+register_dump_files_1 (struct opt_pass *pass)
{
do
{
- int new_properties = (properties | pass->properties_provided)
- & ~pass->properties_destroyed;
-
if (pass->name && pass->name[0] != '*')
register_one_dump_file (pass);
if (pass->sub)
- new_properties = register_dump_files_1 (pass->sub, new_properties);
-
- /* If we have a gate, combine the properties that we could have with
- and without the pass being examined. */
- if (pass->has_gate)
- properties &= new_properties;
- else
- properties = new_properties;
+ register_dump_files_1 (pass->sub);
pass = pass->next;
}
while (pass);
-
- return properties;
}
/* Register the dump files for the pass_manager starting at PASS.
@@ -739,7 +727,7 @@ pass_manager::
register_dump_files (struct opt_pass *pass,int properties)
{
pass->properties_required |= properties;
- register_dump_files_1 (pass, properties);
+ register_dump_files_1 (pass);
}
struct pass_registry
@@ -847,7 +835,7 @@ dump_one_pass (struct opt_pass *pass, int pass_indent)
const char *pn;
bool is_on, is_really_on;
- is_on = pass->has_gate ? pass->gate () : true;
+ is_on = pass->gate ();
is_really_on = override_gate_status (pass, current_function_decl, is_on);
if (pass->static_pass_number <= 0)
@@ -2002,7 +1990,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass)
/* Execute all of the IPA_PASSes in the list. */
if (ipa_pass->type == IPA_PASS
- && ((!pass->has_gate) || pass->gate ())
+ && pass->gate ()
&& ipa_pass->generate_summary)
{
pass_init_dump_file (pass);
@@ -2153,7 +2141,7 @@ execute_one_pass (struct opt_pass *pass)
/* Check whether gate check should be avoided.
User controls the value of the gate through the parameter "gate_status". */
- gate_status = pass->has_gate ? pass->gate () : true;
+ gate_status = pass->gate ();
gate_status = override_gate_status (pass, current_function_decl, gate_status);
/* Override gate with plugin. */
@@ -2210,11 +2198,11 @@ execute_one_pass (struct opt_pass *pass)
timevar_push (pass->tv_id);
/* Do it! */
- if (pass->has_execute)
- {
- todo_after = pass->execute ();
- do_per_function (clear_last_verified, NULL);
- }
+ todo_after = pass->execute ();
+ if (todo_after != TODO_absolutely_nothing)
+ do_per_function (clear_last_verified, NULL);
+ else
+ todo_after &= ~TODO_absolutely_nothing;
/* Stop timevar. */
if (pass->tv_id != TV_NONE)
@@ -2286,7 +2274,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state)
gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
if (pass->type == IPA_PASS
&& ipa_pass->write_summary
- && ((!pass->has_gate) || pass->gate ()))
+ && pass->gate ())
{
/* If a timevar is present, start it. */
if (pass->tv_id)
@@ -2402,7 +2390,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s
gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
if (pass->type == IPA_PASS
&& ipa_pass->write_optimization_summary
- && ((!pass->has_gate) || pass->gate ()))
+ && pass->gate ())
{
/* If a timevar is present, start it. */
if (pass->tv_id)
@@ -2479,7 +2467,7 @@ ipa_read_summaries_1 (struct opt_pass *pass)
gcc_assert (!cfun);
gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
- if ((!pass->has_gate) || pass->gate ())
+ if (pass->gate ())
{
if (pass->type == IPA_PASS && ipa_pass->read_summary)
{
@@ -2530,7 +2518,7 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass)
gcc_assert (!cfun);
gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
- if ((!pass->has_gate) || pass->gate ())
+ if (pass->gate ())
{
if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary)
{
@@ -2608,7 +2596,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass,
{
/* Execute all of the IPA_PASSes in the list. */
if (pass->type == IPA_PASS
- && ((!pass->has_gate) || pass->gate ()))
+ && pass->gate ())
{
struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass;
diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
index 9efee1e..bed639e 100644
--- a/gcc/tree-pass.h
+++ b/gcc/tree-pass.h
@@ -49,11 +49,11 @@ struct pass_data
/* If true, this pass has its own implementation of the opt_pass::gate
method. */
- bool has_gate;
+ bool useless_has_gate;
/* If true, this pass has its own implementation of the opt_pass::execute
method. */
- bool has_execute;
+ bool useless_has_execute;
/* The timevar id associated with this pass. */
/* ??? Ideally would be dynamically assigned. */
@@ -299,6 +299,10 @@ protected:
/* Rebuild the callgraph edges. */
#define TODO_rebuild_cgraph_edges (1 << 22)
+/* Should only be used by opt_pass::execute to tell the pass manager the pass
+ did absolutely nothing. */
+#define TODO_absolutely_nothing 1 << 23
+
/* Internally used in execute_function_todo(). */
#define TODO_update_ssa_any \
(TODO_update_ssa \
--
1.8.4.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] make has_gate and has_execute useless
2013-11-07 16:40 [PATCH] make has_gate and has_execute useless tsaunders
@ 2013-11-07 17:51 ` Jeff Law
2013-11-07 18:38 ` Trevor Saunders
2013-11-08 9:44 ` Richard Biener
1 sibling, 1 reply; 12+ messages in thread
From: Jeff Law @ 2013-11-07 17:51 UTC (permalink / raw)
To: tsaunders, gcc-patches
On 11/07/13 09:00, tsaunders@mozilla.com wrote:
> From: Trevor Saunders <tsaunders@mozilla.com>
>
> Hi,
>
> This is the result of seeing what it would take to get rid of the has_gate and
> has_execute flags on pass_data. It turns out not much, but I wanted
> confirmation this part is ok before I go deal with all the places that
> initialize the fields.
Most definitely, the has_gate/has_execute stuff is definitely a wart.
>
> I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite
> regressions (ignoring the silk stuff because the full paths in its test names
> break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok?
I don't see anything glaringly wrong.
Any reason why the register_dump_files bits were mucking with
properties. That just seems wrong.
> + todo_after = pass->execute ();
> + if (todo_after != TODO_absolutely_nothing)
> + do_per_function (clear_last_verified, NULL);
> + else
> + todo_after &= ~TODO_absolutely_nothing;
Isn't the last assignment there just
todo_after = 0;
?
Jeff
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] make has_gate and has_execute useless
2013-11-07 17:51 ` Jeff Law
@ 2013-11-07 18:38 ` Trevor Saunders
0 siblings, 0 replies; 12+ messages in thread
From: Trevor Saunders @ 2013-11-07 18:38 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches
On Thu, Nov 07, 2013 at 10:30:16AM -0700, Jeff Law wrote:
> On 11/07/13 09:00, tsaunders@mozilla.com wrote:
> >From: Trevor Saunders <tsaunders@mozilla.com>
> >
> >Hi,
> >
> > This is the result of seeing what it would take to get rid of the has_gate and
> >has_execute flags on pass_data. It turns out not much, but I wanted
> >confirmation this part is ok before I go deal with all the places that
> >initialize the fields.
> Most definitely, the has_gate/has_execute stuff is definitely a wart.
>
> >
> >I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite
> >regressions (ignoring the silk stuff because the full paths in its test names
> >break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok?
> I don't see anything glaringly wrong.
great, I'll work on ripping them out then :)
> Any reason why the register_dump_files bits were mucking with
> properties. That just seems wrong.
no, it looks like its pretty old, the most recent change I can see is
r110742 in 2006, but I agree it seems crazy, but atleast its not
actually doing anything now.
> >+ todo_after = pass->execute ();
> >+ if (todo_after != TODO_absolutely_nothing)
> >+ do_per_function (clear_last_verified, NULL);
> >+ else
> >+ todo_after &= ~TODO_absolutely_nothing;
>
> Isn't the last assignment there just
> todo_after = 0;
yes
Trev
>
> ?
>
> Jeff
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] make has_gate and has_execute useless
2013-11-07 16:40 [PATCH] make has_gate and has_execute useless tsaunders
2013-11-07 17:51 ` Jeff Law
@ 2013-11-08 9:44 ` Richard Biener
2013-11-08 15:42 ` Trevor Saunders
` (2 more replies)
1 sibling, 3 replies; 12+ messages in thread
From: Richard Biener @ 2013-11-08 9:44 UTC (permalink / raw)
To: tsaunders; +Cc: GCC Patches
On Thu, Nov 7, 2013 at 5:00 PM, <tsaunders@mozilla.com> wrote:
> From: Trevor Saunders <tsaunders@mozilla.com>
>
> Hi,
>
> This is the result of seeing what it would take to get rid of the has_gate and
> has_execute flags on pass_data. It turns out not much, but I wanted
> confirmation this part is ok before I go deal with all the places that
> initialize the fields.
>
> I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite
> regressions (ignoring the silk stuff because the full paths in its test names
> break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok?
The has_gate flag is easy to remove (without a TODO_ hack), right?
No gate simply means that the gate returns always true. The only
weird thing is
/* If we have a gate, combine the properties that we could have with
and without the pass being examined. */
if (pass->has_gate)
properties &= new_properties;
else
properties = new_properties;
which I don't understand (and you just removed all properties handling there).
So can you split out removing has_gate? This part is obviously ok.
Then, for ->execute () I'd have refactored the code to make
->sub passes explicitely executed by the default ->execute ()
implementation only. That is, passes without overriding execute
are containers only. Can you quickly check whether that would
work out?
Thanks,
Richard.
> Trev
>
> 2013-11-06 Trevor Saunders <tsaunders@mozilla.com>
>
> * pass_manager.h (pass_manager): Adjust.
> * passes.c (opt_pass::execute): Tell the pass manager it doesn't need
> to do anything for this pass.
> (pass_manager::register_dump_files_1): Don't uselessly deal with
> properties of passes.
> (pass_manager::register_dump_files): Adjust.
> (dump_one_pass): Just call pass->gate ().
> (execute_ipa_summary_passes): Likewise.
> (execute_one_pass): Don't check pass->has_execute flag.
> (ipa_write_summaries_2): Don't check pass->has_gate flag.
> (ipa_write_optimization_summaries_1): Likewise.
> (ipa_read_summaries_1): Likewise.
> (ipa_read_optimization_summaries_1): Likewise.
> (execute_ipa_stmt_fixups): Likewise.
> * tree-pass.h (pass_data): Rename has_gate to useless_has_gate and
> has_execute to useless_has_execute to be sure they're unused.
> (TODO_absolutely_nothing): New constant.
>
>
> diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
> index 77d78eb..3bc0a99 100644
> --- a/gcc/pass_manager.h
> +++ b/gcc/pass_manager.h
> @@ -93,7 +93,7 @@ public:
>
> private:
> void set_pass_for_id (int id, opt_pass *pass);
> - int register_dump_files_1 (struct opt_pass *pass, int properties);
> + void register_dump_files_1 (struct opt_pass *pass);
> void register_dump_files (struct opt_pass *pass, int properties);
>
> private:
> diff --git a/gcc/passes.c b/gcc/passes.c
> index 19e5869..3b28dc9 100644
> --- a/gcc/passes.c
> +++ b/gcc/passes.c
> @@ -112,7 +112,7 @@ opt_pass::gate ()
> unsigned int
> opt_pass::execute ()
> {
> - return 0;
> + return TODO_absolutely_nothing;
> }
>
> opt_pass::opt_pass (const pass_data &data, context *ctxt)
> @@ -701,33 +701,21 @@ pass_manager::register_one_dump_file (struct opt_pass *pass)
>
> /* Recursive worker function for register_dump_files. */
>
> -int
> +void
> pass_manager::
> -register_dump_files_1 (struct opt_pass *pass, int properties)
> +register_dump_files_1 (struct opt_pass *pass)
> {
> do
> {
> - int new_properties = (properties | pass->properties_provided)
> - & ~pass->properties_destroyed;
> -
> if (pass->name && pass->name[0] != '*')
> register_one_dump_file (pass);
>
> if (pass->sub)
> - new_properties = register_dump_files_1 (pass->sub, new_properties);
> -
> - /* If we have a gate, combine the properties that we could have with
> - and without the pass being examined. */
> - if (pass->has_gate)
> - properties &= new_properties;
> - else
> - properties = new_properties;
> + register_dump_files_1 (pass->sub);
>
> pass = pass->next;
> }
> while (pass);
> -
> - return properties;
> }
>
> /* Register the dump files for the pass_manager starting at PASS.
> @@ -739,7 +727,7 @@ pass_manager::
> register_dump_files (struct opt_pass *pass,int properties)
> {
> pass->properties_required |= properties;
> - register_dump_files_1 (pass, properties);
> + register_dump_files_1 (pass);
> }
>
> struct pass_registry
> @@ -847,7 +835,7 @@ dump_one_pass (struct opt_pass *pass, int pass_indent)
> const char *pn;
> bool is_on, is_really_on;
>
> - is_on = pass->has_gate ? pass->gate () : true;
> + is_on = pass->gate ();
> is_really_on = override_gate_status (pass, current_function_decl, is_on);
>
> if (pass->static_pass_number <= 0)
> @@ -2002,7 +1990,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass)
>
> /* Execute all of the IPA_PASSes in the list. */
> if (ipa_pass->type == IPA_PASS
> - && ((!pass->has_gate) || pass->gate ())
> + && pass->gate ()
> && ipa_pass->generate_summary)
> {
> pass_init_dump_file (pass);
> @@ -2153,7 +2141,7 @@ execute_one_pass (struct opt_pass *pass)
>
> /* Check whether gate check should be avoided.
> User controls the value of the gate through the parameter "gate_status". */
> - gate_status = pass->has_gate ? pass->gate () : true;
> + gate_status = pass->gate ();
> gate_status = override_gate_status (pass, current_function_decl, gate_status);
>
> /* Override gate with plugin. */
> @@ -2210,11 +2198,11 @@ execute_one_pass (struct opt_pass *pass)
> timevar_push (pass->tv_id);
>
> /* Do it! */
> - if (pass->has_execute)
> - {
> - todo_after = pass->execute ();
> - do_per_function (clear_last_verified, NULL);
> - }
> + todo_after = pass->execute ();
> + if (todo_after != TODO_absolutely_nothing)
> + do_per_function (clear_last_verified, NULL);
> + else
> + todo_after &= ~TODO_absolutely_nothing;
>
> /* Stop timevar. */
> if (pass->tv_id != TV_NONE)
> @@ -2286,7 +2274,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state)
> gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> if (pass->type == IPA_PASS
> && ipa_pass->write_summary
> - && ((!pass->has_gate) || pass->gate ()))
> + && pass->gate ())
> {
> /* If a timevar is present, start it. */
> if (pass->tv_id)
> @@ -2402,7 +2390,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s
> gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> if (pass->type == IPA_PASS
> && ipa_pass->write_optimization_summary
> - && ((!pass->has_gate) || pass->gate ()))
> + && pass->gate ())
> {
> /* If a timevar is present, start it. */
> if (pass->tv_id)
> @@ -2479,7 +2467,7 @@ ipa_read_summaries_1 (struct opt_pass *pass)
> gcc_assert (!cfun);
> gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>
> - if ((!pass->has_gate) || pass->gate ())
> + if (pass->gate ())
> {
> if (pass->type == IPA_PASS && ipa_pass->read_summary)
> {
> @@ -2530,7 +2518,7 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass)
> gcc_assert (!cfun);
> gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>
> - if ((!pass->has_gate) || pass->gate ())
> + if (pass->gate ())
> {
> if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary)
> {
> @@ -2608,7 +2596,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass,
> {
> /* Execute all of the IPA_PASSes in the list. */
> if (pass->type == IPA_PASS
> - && ((!pass->has_gate) || pass->gate ()))
> + && pass->gate ())
> {
> struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass;
>
> diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
> index 9efee1e..bed639e 100644
> --- a/gcc/tree-pass.h
> +++ b/gcc/tree-pass.h
> @@ -49,11 +49,11 @@ struct pass_data
>
> /* If true, this pass has its own implementation of the opt_pass::gate
> method. */
> - bool has_gate;
> + bool useless_has_gate;
>
> /* If true, this pass has its own implementation of the opt_pass::execute
> method. */
> - bool has_execute;
> + bool useless_has_execute;
>
> /* The timevar id associated with this pass. */
> /* ??? Ideally would be dynamically assigned. */
> @@ -299,6 +299,10 @@ protected:
> /* Rebuild the callgraph edges. */
> #define TODO_rebuild_cgraph_edges (1 << 22)
>
> +/* Should only be used by opt_pass::execute to tell the pass manager the pass
> + did absolutely nothing. */
> +#define TODO_absolutely_nothing 1 << 23
> +
> /* Internally used in execute_function_todo(). */
> #define TODO_update_ssa_any \
> (TODO_update_ssa \
> --
> 1.8.4.2
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] make has_gate and has_execute useless
2013-11-08 9:44 ` Richard Biener
@ 2013-11-08 15:42 ` Trevor Saunders
2013-11-11 3:05 ` Trevor Saunders
2013-11-11 18:31 ` Paolo Bonzini
2 siblings, 0 replies; 12+ messages in thread
From: Trevor Saunders @ 2013-11-08 15:42 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
On Fri, Nov 08, 2013 at 10:37:00AM +0100, Richard Biener wrote:
> On Thu, Nov 7, 2013 at 5:00 PM, <tsaunders@mozilla.com> wrote:
> > From: Trevor Saunders <tsaunders@mozilla.com>
> >
> > Hi,
> >
> > This is the result of seeing what it would take to get rid of the has_gate and
> > has_execute flags on pass_data. It turns out not much, but I wanted
> > confirmation this part is ok before I go deal with all the places that
> > initialize the fields.
> >
> > I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite
> > regressions (ignoring the silk stuff because the full paths in its test names
> > break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok?
>
> The has_gate flag is easy to remove (without a TODO_ hack), right?
yes
> No gate simply means that the gate returns always true. The only
> weird thing is
>
> /* If we have a gate, combine the properties that we could have with
> and without the pass being examined. */
> if (pass->has_gate)
> properties &= new_properties;
> else
> properties = new_properties;
>
> which I don't understand (and you just removed all properties handling there).
yes, because it was pretty obviously doing nothing useful, but I'm not sure
what it was trying to do.
> So can you split out removing has_gate? This part is obviously ok.
ugh, already wrote / sent the patch that got rid of setting both flags.
> Then, for ->execute () I'd have refactored the code to make
> ->sub passes explicitely executed by the default ->execute ()
> implementation only. That is, passes without overriding execute
> are containers only. Can you quickly check whether that would
> work out?
It seems nice and I'll try it. A quick look makes me worry a little
about what execute_ipa_pass_list is doing with passes->sub, but I
suspect that's the wrong place for whatever it is, and maybe it isn't
actually an issue.
Trev
>
> Thanks,
> Richard.
>
> > Trev
> >
> > 2013-11-06 Trevor Saunders <tsaunders@mozilla.com>
> >
> > * pass_manager.h (pass_manager): Adjust.
> > * passes.c (opt_pass::execute): Tell the pass manager it doesn't need
> > to do anything for this pass.
> > (pass_manager::register_dump_files_1): Don't uselessly deal with
> > properties of passes.
> > (pass_manager::register_dump_files): Adjust.
> > (dump_one_pass): Just call pass->gate ().
> > (execute_ipa_summary_passes): Likewise.
> > (execute_one_pass): Don't check pass->has_execute flag.
> > (ipa_write_summaries_2): Don't check pass->has_gate flag.
> > (ipa_write_optimization_summaries_1): Likewise.
> > (ipa_read_summaries_1): Likewise.
> > (ipa_read_optimization_summaries_1): Likewise.
> > (execute_ipa_stmt_fixups): Likewise.
> > * tree-pass.h (pass_data): Rename has_gate to useless_has_gate and
> > has_execute to useless_has_execute to be sure they're unused.
> > (TODO_absolutely_nothing): New constant.
> >
> >
> > diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
> > index 77d78eb..3bc0a99 100644
> > --- a/gcc/pass_manager.h
> > +++ b/gcc/pass_manager.h
> > @@ -93,7 +93,7 @@ public:
> >
> > private:
> > void set_pass_for_id (int id, opt_pass *pass);
> > - int register_dump_files_1 (struct opt_pass *pass, int properties);
> > + void register_dump_files_1 (struct opt_pass *pass);
> > void register_dump_files (struct opt_pass *pass, int properties);
> >
> > private:
> > diff --git a/gcc/passes.c b/gcc/passes.c
> > index 19e5869..3b28dc9 100644
> > --- a/gcc/passes.c
> > +++ b/gcc/passes.c
> > @@ -112,7 +112,7 @@ opt_pass::gate ()
> > unsigned int
> > opt_pass::execute ()
> > {
> > - return 0;
> > + return TODO_absolutely_nothing;
> > }
> >
> > opt_pass::opt_pass (const pass_data &data, context *ctxt)
> > @@ -701,33 +701,21 @@ pass_manager::register_one_dump_file (struct opt_pass *pass)
> >
> > /* Recursive worker function for register_dump_files. */
> >
> > -int
> > +void
> > pass_manager::
> > -register_dump_files_1 (struct opt_pass *pass, int properties)
> > +register_dump_files_1 (struct opt_pass *pass)
> > {
> > do
> > {
> > - int new_properties = (properties | pass->properties_provided)
> > - & ~pass->properties_destroyed;
> > -
> > if (pass->name && pass->name[0] != '*')
> > register_one_dump_file (pass);
> >
> > if (pass->sub)
> > - new_properties = register_dump_files_1 (pass->sub, new_properties);
> > -
> > - /* If we have a gate, combine the properties that we could have with
> > - and without the pass being examined. */
> > - if (pass->has_gate)
> > - properties &= new_properties;
> > - else
> > - properties = new_properties;
> > + register_dump_files_1 (pass->sub);
> >
> > pass = pass->next;
> > }
> > while (pass);
> > -
> > - return properties;
> > }
> >
> > /* Register the dump files for the pass_manager starting at PASS.
> > @@ -739,7 +727,7 @@ pass_manager::
> > register_dump_files (struct opt_pass *pass,int properties)
> > {
> > pass->properties_required |= properties;
> > - register_dump_files_1 (pass, properties);
> > + register_dump_files_1 (pass);
> > }
> >
> > struct pass_registry
> > @@ -847,7 +835,7 @@ dump_one_pass (struct opt_pass *pass, int pass_indent)
> > const char *pn;
> > bool is_on, is_really_on;
> >
> > - is_on = pass->has_gate ? pass->gate () : true;
> > + is_on = pass->gate ();
> > is_really_on = override_gate_status (pass, current_function_decl, is_on);
> >
> > if (pass->static_pass_number <= 0)
> > @@ -2002,7 +1990,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass)
> >
> > /* Execute all of the IPA_PASSes in the list. */
> > if (ipa_pass->type == IPA_PASS
> > - && ((!pass->has_gate) || pass->gate ())
> > + && pass->gate ()
> > && ipa_pass->generate_summary)
> > {
> > pass_init_dump_file (pass);
> > @@ -2153,7 +2141,7 @@ execute_one_pass (struct opt_pass *pass)
> >
> > /* Check whether gate check should be avoided.
> > User controls the value of the gate through the parameter "gate_status". */
> > - gate_status = pass->has_gate ? pass->gate () : true;
> > + gate_status = pass->gate ();
> > gate_status = override_gate_status (pass, current_function_decl, gate_status);
> >
> > /* Override gate with plugin. */
> > @@ -2210,11 +2198,11 @@ execute_one_pass (struct opt_pass *pass)
> > timevar_push (pass->tv_id);
> >
> > /* Do it! */
> > - if (pass->has_execute)
> > - {
> > - todo_after = pass->execute ();
> > - do_per_function (clear_last_verified, NULL);
> > - }
> > + todo_after = pass->execute ();
> > + if (todo_after != TODO_absolutely_nothing)
> > + do_per_function (clear_last_verified, NULL);
> > + else
> > + todo_after &= ~TODO_absolutely_nothing;
> >
> > /* Stop timevar. */
> > if (pass->tv_id != TV_NONE)
> > @@ -2286,7 +2274,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state)
> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> > if (pass->type == IPA_PASS
> > && ipa_pass->write_summary
> > - && ((!pass->has_gate) || pass->gate ()))
> > + && pass->gate ())
> > {
> > /* If a timevar is present, start it. */
> > if (pass->tv_id)
> > @@ -2402,7 +2390,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s
> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> > if (pass->type == IPA_PASS
> > && ipa_pass->write_optimization_summary
> > - && ((!pass->has_gate) || pass->gate ()))
> > + && pass->gate ())
> > {
> > /* If a timevar is present, start it. */
> > if (pass->tv_id)
> > @@ -2479,7 +2467,7 @@ ipa_read_summaries_1 (struct opt_pass *pass)
> > gcc_assert (!cfun);
> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> >
> > - if ((!pass->has_gate) || pass->gate ())
> > + if (pass->gate ())
> > {
> > if (pass->type == IPA_PASS && ipa_pass->read_summary)
> > {
> > @@ -2530,7 +2518,7 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass)
> > gcc_assert (!cfun);
> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> >
> > - if ((!pass->has_gate) || pass->gate ())
> > + if (pass->gate ())
> > {
> > if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary)
> > {
> > @@ -2608,7 +2596,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass,
> > {
> > /* Execute all of the IPA_PASSes in the list. */
> > if (pass->type == IPA_PASS
> > - && ((!pass->has_gate) || pass->gate ()))
> > + && pass->gate ())
> > {
> > struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass;
> >
> > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
> > index 9efee1e..bed639e 100644
> > --- a/gcc/tree-pass.h
> > +++ b/gcc/tree-pass.h
> > @@ -49,11 +49,11 @@ struct pass_data
> >
> > /* If true, this pass has its own implementation of the opt_pass::gate
> > method. */
> > - bool has_gate;
> > + bool useless_has_gate;
> >
> > /* If true, this pass has its own implementation of the opt_pass::execute
> > method. */
> > - bool has_execute;
> > + bool useless_has_execute;
> >
> > /* The timevar id associated with this pass. */
> > /* ??? Ideally would be dynamically assigned. */
> > @@ -299,6 +299,10 @@ protected:
> > /* Rebuild the callgraph edges. */
> > #define TODO_rebuild_cgraph_edges (1 << 22)
> >
> > +/* Should only be used by opt_pass::execute to tell the pass manager the pass
> > + did absolutely nothing. */
> > +#define TODO_absolutely_nothing 1 << 23
> > +
> > /* Internally used in execute_function_todo(). */
> > #define TODO_update_ssa_any \
> > (TODO_update_ssa \
> > --
> > 1.8.4.2
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] make has_gate and has_execute useless
2013-11-08 9:44 ` Richard Biener
2013-11-08 15:42 ` Trevor Saunders
@ 2013-11-11 3:05 ` Trevor Saunders
2013-11-11 12:16 ` Richard Biener
2013-11-11 18:31 ` Paolo Bonzini
2 siblings, 1 reply; 12+ messages in thread
From: Trevor Saunders @ 2013-11-11 3:05 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
On Fri, Nov 08, 2013 at 10:37:00AM +0100, Richard Biener wrote:
> On Thu, Nov 7, 2013 at 5:00 PM, <tsaunders@mozilla.com> wrote:
> > From: Trevor Saunders <tsaunders@mozilla.com>
> >
> > Hi,
> >
> > This is the result of seeing what it would take to get rid of the has_gate and
> > has_execute flags on pass_data. It turns out not much, but I wanted
> > confirmation this part is ok before I go deal with all the places that
> > initialize the fields.
> >
> > I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite
> > regressions (ignoring the silk stuff because the full paths in its test names
> > break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok?
>
> The has_gate flag is easy to remove (without a TODO_ hack), right?
> No gate simply means that the gate returns always true. The only
> weird thing is
>
> /* If we have a gate, combine the properties that we could have with
> and without the pass being examined. */
> if (pass->has_gate)
> properties &= new_properties;
> else
> properties = new_properties;
>
> which I don't understand (and you just removed all properties handling there).
>
> So can you split out removing has_gate? This part is obviously ok.
>
> Then, for ->execute () I'd have refactored the code to make
> ->sub passes explicitely executed by the default ->execute ()
> implementation only. That is, passes without overriding execute
> are containers only. Can you quickly check whether that would
> work out?
Ok, I've now given this a shot and wasn't terribly successful, if I just
change execute_pass_list and execute_ipa_pass_list to handle container
passes executing their sub passes I get the following ICE
./gt-passes.h:77:2: internal compiler error: Segmentation fault
};
^
0xd43d96 crash_signal
/src/gcc/gcc/toplev.c:334
0xd901a9 ssa_default_def(function*, tree_node*)
/src/gcc/gcc/tree-dfa.c:318
0xb56d77 ipa_analyze_params_uses
/src/gcc/gcc/ipa-prop.c:2094
0xb57275 ipa_analyze_node(cgraph_node*)
/src/gcc/gcc/ipa-prop.c:2179
0x13e2b6d ipcp_generate_summary
/src/gcc/gcc/ipa-cp.c:3615
0xc55a2a
execute_ipa_summary_passes(ipa_opt_pass_d*)
/src/gcc/gcc/passes.c:1991
0x943341 ipa_passes
/src/gcc/gcc/cgraphunit.c:2011
0x943675
compile()
/src/gcc/gcc/cgraphunit.c:2118
now
Which is because fn->gimple_df is null. I expect that's because pass
ordering changed somehow, but I'm not sure off hand how or ifthat's
something worth figuring out right now.
Another issue I realized is that doing this will change the order of
plugin events from
start container pass a
end container pass a
start contained pass b
end contained pass b
...
to
start container pass a
start contained pass b
end contained pass b
end container pass a
Arguably that's an improvement, but I'm not sure if changing that APi
like that is acceptable.
Trev
>
> Thanks,
> Richard.
>
> > Trev
> >
> > 2013-11-06 Trevor Saunders <tsaunders@mozilla.com>
> >
> > * pass_manager.h (pass_manager): Adjust.
> > * passes.c (opt_pass::execute): Tell the pass manager it doesn't need
> > to do anything for this pass.
> > (pass_manager::register_dump_files_1): Don't uselessly deal with
> > properties of passes.
> > (pass_manager::register_dump_files): Adjust.
> > (dump_one_pass): Just call pass->gate ().
> > (execute_ipa_summary_passes): Likewise.
> > (execute_one_pass): Don't check pass->has_execute flag.
> > (ipa_write_summaries_2): Don't check pass->has_gate flag.
> > (ipa_write_optimization_summaries_1): Likewise.
> > (ipa_read_summaries_1): Likewise.
> > (ipa_read_optimization_summaries_1): Likewise.
> > (execute_ipa_stmt_fixups): Likewise.
> > * tree-pass.h (pass_data): Rename has_gate to useless_has_gate and
> > has_execute to useless_has_execute to be sure they're unused.
> > (TODO_absolutely_nothing): New constant.
> >
> >
> > diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
> > index 77d78eb..3bc0a99 100644
> > --- a/gcc/pass_manager.h
> > +++ b/gcc/pass_manager.h
> > @@ -93,7 +93,7 @@ public:
> >
> > private:
> > void set_pass_for_id (int id, opt_pass *pass);
> > - int register_dump_files_1 (struct opt_pass *pass, int properties);
> > + void register_dump_files_1 (struct opt_pass *pass);
> > void register_dump_files (struct opt_pass *pass, int properties);
> >
> > private:
> > diff --git a/gcc/passes.c b/gcc/passes.c
> > index 19e5869..3b28dc9 100644
> > --- a/gcc/passes.c
> > +++ b/gcc/passes.c
> > @@ -112,7 +112,7 @@ opt_pass::gate ()
> > unsigned int
> > opt_pass::execute ()
> > {
> > - return 0;
> > + return TODO_absolutely_nothing;
> > }
> >
> > opt_pass::opt_pass (const pass_data &data, context *ctxt)
> > @@ -701,33 +701,21 @@ pass_manager::register_one_dump_file (struct opt_pass *pass)
> >
> > /* Recursive worker function for register_dump_files. */
> >
> > -int
> > +void
> > pass_manager::
> > -register_dump_files_1 (struct opt_pass *pass, int properties)
> > +register_dump_files_1 (struct opt_pass *pass)
> > {
> > do
> > {
> > - int new_properties = (properties | pass->properties_provided)
> > - & ~pass->properties_destroyed;
> > -
> > if (pass->name && pass->name[0] != '*')
> > register_one_dump_file (pass);
> >
> > if (pass->sub)
> > - new_properties = register_dump_files_1 (pass->sub, new_properties);
> > -
> > - /* If we have a gate, combine the properties that we could have with
> > - and without the pass being examined. */
> > - if (pass->has_gate)
> > - properties &= new_properties;
> > - else
> > - properties = new_properties;
> > + register_dump_files_1 (pass->sub);
> >
> > pass = pass->next;
> > }
> > while (pass);
> > -
> > - return properties;
> > }
> >
> > /* Register the dump files for the pass_manager starting at PASS.
> > @@ -739,7 +727,7 @@ pass_manager::
> > register_dump_files (struct opt_pass *pass,int properties)
> > {
> > pass->properties_required |= properties;
> > - register_dump_files_1 (pass, properties);
> > + register_dump_files_1 (pass);
> > }
> >
> > struct pass_registry
> > @@ -847,7 +835,7 @@ dump_one_pass (struct opt_pass *pass, int pass_indent)
> > const char *pn;
> > bool is_on, is_really_on;
> >
> > - is_on = pass->has_gate ? pass->gate () : true;
> > + is_on = pass->gate ();
> > is_really_on = override_gate_status (pass, current_function_decl, is_on);
> >
> > if (pass->static_pass_number <= 0)
> > @@ -2002,7 +1990,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass)
> >
> > /* Execute all of the IPA_PASSes in the list. */
> > if (ipa_pass->type == IPA_PASS
> > - && ((!pass->has_gate) || pass->gate ())
> > + && pass->gate ()
> > && ipa_pass->generate_summary)
> > {
> > pass_init_dump_file (pass);
> > @@ -2153,7 +2141,7 @@ execute_one_pass (struct opt_pass *pass)
> >
> > /* Check whether gate check should be avoided.
> > User controls the value of the gate through the parameter "gate_status". */
> > - gate_status = pass->has_gate ? pass->gate () : true;
> > + gate_status = pass->gate ();
> > gate_status = override_gate_status (pass, current_function_decl, gate_status);
> >
> > /* Override gate with plugin. */
> > @@ -2210,11 +2198,11 @@ execute_one_pass (struct opt_pass *pass)
> > timevar_push (pass->tv_id);
> >
> > /* Do it! */
> > - if (pass->has_execute)
> > - {
> > - todo_after = pass->execute ();
> > - do_per_function (clear_last_verified, NULL);
> > - }
> > + todo_after = pass->execute ();
> > + if (todo_after != TODO_absolutely_nothing)
> > + do_per_function (clear_last_verified, NULL);
> > + else
> > + todo_after &= ~TODO_absolutely_nothing;
> >
> > /* Stop timevar. */
> > if (pass->tv_id != TV_NONE)
> > @@ -2286,7 +2274,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state)
> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> > if (pass->type == IPA_PASS
> > && ipa_pass->write_summary
> > - && ((!pass->has_gate) || pass->gate ()))
> > + && pass->gate ())
> > {
> > /* If a timevar is present, start it. */
> > if (pass->tv_id)
> > @@ -2402,7 +2390,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s
> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> > if (pass->type == IPA_PASS
> > && ipa_pass->write_optimization_summary
> > - && ((!pass->has_gate) || pass->gate ()))
> > + && pass->gate ())
> > {
> > /* If a timevar is present, start it. */
> > if (pass->tv_id)
> > @@ -2479,7 +2467,7 @@ ipa_read_summaries_1 (struct opt_pass *pass)
> > gcc_assert (!cfun);
> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> >
> > - if ((!pass->has_gate) || pass->gate ())
> > + if (pass->gate ())
> > {
> > if (pass->type == IPA_PASS && ipa_pass->read_summary)
> > {
> > @@ -2530,7 +2518,7 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass)
> > gcc_assert (!cfun);
> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> >
> > - if ((!pass->has_gate) || pass->gate ())
> > + if (pass->gate ())
> > {
> > if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary)
> > {
> > @@ -2608,7 +2596,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass,
> > {
> > /* Execute all of the IPA_PASSes in the list. */
> > if (pass->type == IPA_PASS
> > - && ((!pass->has_gate) || pass->gate ()))
> > + && pass->gate ())
> > {
> > struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass;
> >
> > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
> > index 9efee1e..bed639e 100644
> > --- a/gcc/tree-pass.h
> > +++ b/gcc/tree-pass.h
> > @@ -49,11 +49,11 @@ struct pass_data
> >
> > /* If true, this pass has its own implementation of the opt_pass::gate
> > method. */
> > - bool has_gate;
> > + bool useless_has_gate;
> >
> > /* If true, this pass has its own implementation of the opt_pass::execute
> > method. */
> > - bool has_execute;
> > + bool useless_has_execute;
> >
> > /* The timevar id associated with this pass. */
> > /* ??? Ideally would be dynamically assigned. */
> > @@ -299,6 +299,10 @@ protected:
> > /* Rebuild the callgraph edges. */
> > #define TODO_rebuild_cgraph_edges (1 << 22)
> >
> > +/* Should only be used by opt_pass::execute to tell the pass manager the pass
> > + did absolutely nothing. */
> > +#define TODO_absolutely_nothing 1 << 23
> > +
> > /* Internally used in execute_function_todo(). */
> > #define TODO_update_ssa_any \
> > (TODO_update_ssa \
> > --
> > 1.8.4.2
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] make has_gate and has_execute useless
2013-11-11 3:05 ` Trevor Saunders
@ 2013-11-11 12:16 ` Richard Biener
2013-11-11 17:09 ` Trevor Saunders
0 siblings, 1 reply; 12+ messages in thread
From: Richard Biener @ 2013-11-11 12:16 UTC (permalink / raw)
To: Trevor Saunders; +Cc: GCC Patches
On Mon, Nov 11, 2013 at 1:39 AM, Trevor Saunders <tsaunders@mozilla.com> wrote:
> On Fri, Nov 08, 2013 at 10:37:00AM +0100, Richard Biener wrote:
>> On Thu, Nov 7, 2013 at 5:00 PM, <tsaunders@mozilla.com> wrote:
>> > From: Trevor Saunders <tsaunders@mozilla.com>
>> >
>> > Hi,
>> >
>> > This is the result of seeing what it would take to get rid of the has_gate and
>> > has_execute flags on pass_data. It turns out not much, but I wanted
>> > confirmation this part is ok before I go deal with all the places that
>> > initialize the fields.
>> >
>> > I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite
>> > regressions (ignoring the silk stuff because the full paths in its test names
>> > break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok?
>>
>> The has_gate flag is easy to remove (without a TODO_ hack), right?
>> No gate simply means that the gate returns always true. The only
>> weird thing is
>>
>> /* If we have a gate, combine the properties that we could have with
>> and without the pass being examined. */
>> if (pass->has_gate)
>> properties &= new_properties;
>> else
>> properties = new_properties;
>>
>> which I don't understand (and you just removed all properties handling there).
>>
>> So can you split out removing has_gate? This part is obviously ok.
>>
>> Then, for ->execute () I'd have refactored the code to make
>> ->sub passes explicitely executed by the default ->execute ()
>> implementation only. That is, passes without overriding execute
>> are containers only. Can you quickly check whether that would
>> work out?
>
> Ok, I've now given this a shot and wasn't terribly successful, if I just
> change execute_pass_list and execute_ipa_pass_list to handle container
> passes executing their sub passes I get the following ICE
>
> ./gt-passes.h:77:2: internal compiler error: Segmentation fault
> };
> ^
> 0xd43d96 crash_signal
> /src/gcc/gcc/toplev.c:334
> 0xd901a9 ssa_default_def(function*, tree_node*)
> /src/gcc/gcc/tree-dfa.c:318
> 0xb56d77 ipa_analyze_params_uses
> /src/gcc/gcc/ipa-prop.c:2094
> 0xb57275 ipa_analyze_node(cgraph_node*)
> /src/gcc/gcc/ipa-prop.c:2179
> 0x13e2b6d ipcp_generate_summary
> /src/gcc/gcc/ipa-cp.c:3615
> 0xc55a2a
> execute_ipa_summary_passes(ipa_opt_pass_d*)
> /src/gcc/gcc/passes.c:1991
> 0x943341 ipa_passes
> /src/gcc/gcc/cgraphunit.c:2011
> 0x943675
> compile()
> /src/gcc/gcc/cgraphunit.c:2118
>
> now
> Which is because fn->gimple_df is null. I expect that's because pass
> ordering changed somehow, but I'm not sure off hand how or ifthat's
> something worth figuring out right now.
Yeah, some of the walking doesn't seem to work ... probably a
pass with sub-passes already has an execute member function ;)
> Another issue I realized is that doing this will change the order of
> plugin events from
> start container pass a
> end container pass a
> start contained pass b
> end contained pass b
> ...
>
> to
>
> start container pass a
> start contained pass b
> end contained pass b
> end container pass a
>
> Arguably that's an improvement, but I'm not sure if changing that APi
> like that is acceptable.
Indeed an improvement. Can you attach your patch?
Thanks,
Richard.
> Trev
>
>>
>> Thanks,
>> Richard.
>>
>> > Trev
>> >
>> > 2013-11-06 Trevor Saunders <tsaunders@mozilla.com>
>> >
>> > * pass_manager.h (pass_manager): Adjust.
>> > * passes.c (opt_pass::execute): Tell the pass manager it doesn't need
>> > to do anything for this pass.
>> > (pass_manager::register_dump_files_1): Don't uselessly deal with
>> > properties of passes.
>> > (pass_manager::register_dump_files): Adjust.
>> > (dump_one_pass): Just call pass->gate ().
>> > (execute_ipa_summary_passes): Likewise.
>> > (execute_one_pass): Don't check pass->has_execute flag.
>> > (ipa_write_summaries_2): Don't check pass->has_gate flag.
>> > (ipa_write_optimization_summaries_1): Likewise.
>> > (ipa_read_summaries_1): Likewise.
>> > (ipa_read_optimization_summaries_1): Likewise.
>> > (execute_ipa_stmt_fixups): Likewise.
>> > * tree-pass.h (pass_data): Rename has_gate to useless_has_gate and
>> > has_execute to useless_has_execute to be sure they're unused.
>> > (TODO_absolutely_nothing): New constant.
>> >
>> >
>> > diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
>> > index 77d78eb..3bc0a99 100644
>> > --- a/gcc/pass_manager.h
>> > +++ b/gcc/pass_manager.h
>> > @@ -93,7 +93,7 @@ public:
>> >
>> > private:
>> > void set_pass_for_id (int id, opt_pass *pass);
>> > - int register_dump_files_1 (struct opt_pass *pass, int properties);
>> > + void register_dump_files_1 (struct opt_pass *pass);
>> > void register_dump_files (struct opt_pass *pass, int properties);
>> >
>> > private:
>> > diff --git a/gcc/passes.c b/gcc/passes.c
>> > index 19e5869..3b28dc9 100644
>> > --- a/gcc/passes.c
>> > +++ b/gcc/passes.c
>> > @@ -112,7 +112,7 @@ opt_pass::gate ()
>> > unsigned int
>> > opt_pass::execute ()
>> > {
>> > - return 0;
>> > + return TODO_absolutely_nothing;
>> > }
>> >
>> > opt_pass::opt_pass (const pass_data &data, context *ctxt)
>> > @@ -701,33 +701,21 @@ pass_manager::register_one_dump_file (struct opt_pass *pass)
>> >
>> > /* Recursive worker function for register_dump_files. */
>> >
>> > -int
>> > +void
>> > pass_manager::
>> > -register_dump_files_1 (struct opt_pass *pass, int properties)
>> > +register_dump_files_1 (struct opt_pass *pass)
>> > {
>> > do
>> > {
>> > - int new_properties = (properties | pass->properties_provided)
>> > - & ~pass->properties_destroyed;
>> > -
>> > if (pass->name && pass->name[0] != '*')
>> > register_one_dump_file (pass);
>> >
>> > if (pass->sub)
>> > - new_properties = register_dump_files_1 (pass->sub, new_properties);
>> > -
>> > - /* If we have a gate, combine the properties that we could have with
>> > - and without the pass being examined. */
>> > - if (pass->has_gate)
>> > - properties &= new_properties;
>> > - else
>> > - properties = new_properties;
>> > + register_dump_files_1 (pass->sub);
>> >
>> > pass = pass->next;
>> > }
>> > while (pass);
>> > -
>> > - return properties;
>> > }
>> >
>> > /* Register the dump files for the pass_manager starting at PASS.
>> > @@ -739,7 +727,7 @@ pass_manager::
>> > register_dump_files (struct opt_pass *pass,int properties)
>> > {
>> > pass->properties_required |= properties;
>> > - register_dump_files_1 (pass, properties);
>> > + register_dump_files_1 (pass);
>> > }
>> >
>> > struct pass_registry
>> > @@ -847,7 +835,7 @@ dump_one_pass (struct opt_pass *pass, int pass_indent)
>> > const char *pn;
>> > bool is_on, is_really_on;
>> >
>> > - is_on = pass->has_gate ? pass->gate () : true;
>> > + is_on = pass->gate ();
>> > is_really_on = override_gate_status (pass, current_function_decl, is_on);
>> >
>> > if (pass->static_pass_number <= 0)
>> > @@ -2002,7 +1990,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass)
>> >
>> > /* Execute all of the IPA_PASSes in the list. */
>> > if (ipa_pass->type == IPA_PASS
>> > - && ((!pass->has_gate) || pass->gate ())
>> > + && pass->gate ()
>> > && ipa_pass->generate_summary)
>> > {
>> > pass_init_dump_file (pass);
>> > @@ -2153,7 +2141,7 @@ execute_one_pass (struct opt_pass *pass)
>> >
>> > /* Check whether gate check should be avoided.
>> > User controls the value of the gate through the parameter "gate_status". */
>> > - gate_status = pass->has_gate ? pass->gate () : true;
>> > + gate_status = pass->gate ();
>> > gate_status = override_gate_status (pass, current_function_decl, gate_status);
>> >
>> > /* Override gate with plugin. */
>> > @@ -2210,11 +2198,11 @@ execute_one_pass (struct opt_pass *pass)
>> > timevar_push (pass->tv_id);
>> >
>> > /* Do it! */
>> > - if (pass->has_execute)
>> > - {
>> > - todo_after = pass->execute ();
>> > - do_per_function (clear_last_verified, NULL);
>> > - }
>> > + todo_after = pass->execute ();
>> > + if (todo_after != TODO_absolutely_nothing)
>> > + do_per_function (clear_last_verified, NULL);
>> > + else
>> > + todo_after &= ~TODO_absolutely_nothing;
>> >
>> > /* Stop timevar. */
>> > if (pass->tv_id != TV_NONE)
>> > @@ -2286,7 +2274,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state)
>> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>> > if (pass->type == IPA_PASS
>> > && ipa_pass->write_summary
>> > - && ((!pass->has_gate) || pass->gate ()))
>> > + && pass->gate ())
>> > {
>> > /* If a timevar is present, start it. */
>> > if (pass->tv_id)
>> > @@ -2402,7 +2390,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s
>> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>> > if (pass->type == IPA_PASS
>> > && ipa_pass->write_optimization_summary
>> > - && ((!pass->has_gate) || pass->gate ()))
>> > + && pass->gate ())
>> > {
>> > /* If a timevar is present, start it. */
>> > if (pass->tv_id)
>> > @@ -2479,7 +2467,7 @@ ipa_read_summaries_1 (struct opt_pass *pass)
>> > gcc_assert (!cfun);
>> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>> >
>> > - if ((!pass->has_gate) || pass->gate ())
>> > + if (pass->gate ())
>> > {
>> > if (pass->type == IPA_PASS && ipa_pass->read_summary)
>> > {
>> > @@ -2530,7 +2518,7 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass)
>> > gcc_assert (!cfun);
>> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>> >
>> > - if ((!pass->has_gate) || pass->gate ())
>> > + if (pass->gate ())
>> > {
>> > if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary)
>> > {
>> > @@ -2608,7 +2596,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass,
>> > {
>> > /* Execute all of the IPA_PASSes in the list. */
>> > if (pass->type == IPA_PASS
>> > - && ((!pass->has_gate) || pass->gate ()))
>> > + && pass->gate ())
>> > {
>> > struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass;
>> >
>> > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
>> > index 9efee1e..bed639e 100644
>> > --- a/gcc/tree-pass.h
>> > +++ b/gcc/tree-pass.h
>> > @@ -49,11 +49,11 @@ struct pass_data
>> >
>> > /* If true, this pass has its own implementation of the opt_pass::gate
>> > method. */
>> > - bool has_gate;
>> > + bool useless_has_gate;
>> >
>> > /* If true, this pass has its own implementation of the opt_pass::execute
>> > method. */
>> > - bool has_execute;
>> > + bool useless_has_execute;
>> >
>> > /* The timevar id associated with this pass. */
>> > /* ??? Ideally would be dynamically assigned. */
>> > @@ -299,6 +299,10 @@ protected:
>> > /* Rebuild the callgraph edges. */
>> > #define TODO_rebuild_cgraph_edges (1 << 22)
>> >
>> > +/* Should only be used by opt_pass::execute to tell the pass manager the pass
>> > + did absolutely nothing. */
>> > +#define TODO_absolutely_nothing 1 << 23
>> > +
>> > /* Internally used in execute_function_todo(). */
>> > #define TODO_update_ssa_any \
>> > (TODO_update_ssa \
>> > --
>> > 1.8.4.2
>> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] make has_gate and has_execute useless
2013-11-11 12:16 ` Richard Biener
@ 2013-11-11 17:09 ` Trevor Saunders
2013-11-15 13:10 ` Richard Biener
0 siblings, 1 reply; 12+ messages in thread
From: Trevor Saunders @ 2013-11-11 17:09 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 13298 bytes --]
On Mon, Nov 11, 2013 at 12:58:36PM +0100, Richard Biener wrote:
> On Mon, Nov 11, 2013 at 1:39 AM, Trevor Saunders <tsaunders@mozilla.com> wrote:
> > On Fri, Nov 08, 2013 at 10:37:00AM +0100, Richard Biener wrote:
> >> On Thu, Nov 7, 2013 at 5:00 PM, <tsaunders@mozilla.com> wrote:
> >> > From: Trevor Saunders <tsaunders@mozilla.com>
> >> >
> >> > Hi,
> >> >
> >> > This is the result of seeing what it would take to get rid of the has_gate and
> >> > has_execute flags on pass_data. It turns out not much, but I wanted
> >> > confirmation this part is ok before I go deal with all the places that
> >> > initialize the fields.
> >> >
> >> > I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite
> >> > regressions (ignoring the silk stuff because the full paths in its test names
> >> > break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok?
> >>
> >> The has_gate flag is easy to remove (without a TODO_ hack), right?
> >> No gate simply means that the gate returns always true. The only
> >> weird thing is
> >>
> >> /* If we have a gate, combine the properties that we could have with
> >> and without the pass being examined. */
> >> if (pass->has_gate)
> >> properties &= new_properties;
> >> else
> >> properties = new_properties;
> >>
> >> which I don't understand (and you just removed all properties handling there).
> >>
> >> So can you split out removing has_gate? This part is obviously ok.
> >>
> >> Then, for ->execute () I'd have refactored the code to make
> >> ->sub passes explicitely executed by the default ->execute ()
> >> implementation only. That is, passes without overriding execute
> >> are containers only. Can you quickly check whether that would
> >> work out?
> >
> > Ok, I've now given this a shot and wasn't terribly successful, if I just
> > change execute_pass_list and execute_ipa_pass_list to handle container
> > passes executing their sub passes I get the following ICE
> >
> > ./gt-passes.h:77:2: internal compiler error: Segmentation fault
> > };
> > ^
> > 0xd43d96 crash_signal
> > /src/gcc/gcc/toplev.c:334
> > 0xd901a9 ssa_default_def(function*, tree_node*)
> > /src/gcc/gcc/tree-dfa.c:318
> > 0xb56d77 ipa_analyze_params_uses
> > /src/gcc/gcc/ipa-prop.c:2094
> > 0xb57275 ipa_analyze_node(cgraph_node*)
> > /src/gcc/gcc/ipa-prop.c:2179
> > 0x13e2b6d ipcp_generate_summary
> > /src/gcc/gcc/ipa-cp.c:3615
> > 0xc55a2a
> > execute_ipa_summary_passes(ipa_opt_pass_d*)
> > /src/gcc/gcc/passes.c:1991
> > 0x943341 ipa_passes
> > /src/gcc/gcc/cgraphunit.c:2011
> > 0x943675
> > compile()
> > /src/gcc/gcc/cgraphunit.c:2118
> >
> > now
> > Which is because fn->gimple_df is null. I expect that's because pass
> > ordering changed somehow, but I'm not sure off hand how or ifthat's
> > something worth figuring out right now.
>
> Yeah, some of the walking doesn't seem to work ... probably a
> pass with sub-passes already has an execute member function ;)
Sounds like a reasonable guess.
> > Another issue I realized is that doing this will change the order of
> > plugin events from
> > start container pass a
> > end container pass a
> > start contained pass b
> > end contained pass b
> > ...
> >
> > to
> >
> > start container pass a
> > start contained pass b
> > end contained pass b
> > end container pass a
> >
> > Arguably that's an improvement, but I'm not sure if changing that APi
> > like that is acceptable.
>
> Indeed an improvement. Can you attach your patch?
ok, done
thanks
Trev
>
> Thanks,
> Richard.
>
> > Trev
> >
> >>
> >> Thanks,
> >> Richard.
> >>
> >> > Trev
> >> >
> >> > 2013-11-06 Trevor Saunders <tsaunders@mozilla.com>
> >> >
> >> > * pass_manager.h (pass_manager): Adjust.
> >> > * passes.c (opt_pass::execute): Tell the pass manager it doesn't need
> >> > to do anything for this pass.
> >> > (pass_manager::register_dump_files_1): Don't uselessly deal with
> >> > properties of passes.
> >> > (pass_manager::register_dump_files): Adjust.
> >> > (dump_one_pass): Just call pass->gate ().
> >> > (execute_ipa_summary_passes): Likewise.
> >> > (execute_one_pass): Don't check pass->has_execute flag.
> >> > (ipa_write_summaries_2): Don't check pass->has_gate flag.
> >> > (ipa_write_optimization_summaries_1): Likewise.
> >> > (ipa_read_summaries_1): Likewise.
> >> > (ipa_read_optimization_summaries_1): Likewise.
> >> > (execute_ipa_stmt_fixups): Likewise.
> >> > * tree-pass.h (pass_data): Rename has_gate to useless_has_gate and
> >> > has_execute to useless_has_execute to be sure they're unused.
> >> > (TODO_absolutely_nothing): New constant.
> >> >
> >> >
> >> > diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
> >> > index 77d78eb..3bc0a99 100644
> >> > --- a/gcc/pass_manager.h
> >> > +++ b/gcc/pass_manager.h
> >> > @@ -93,7 +93,7 @@ public:
> >> >
> >> > private:
> >> > void set_pass_for_id (int id, opt_pass *pass);
> >> > - int register_dump_files_1 (struct opt_pass *pass, int properties);
> >> > + void register_dump_files_1 (struct opt_pass *pass);
> >> > void register_dump_files (struct opt_pass *pass, int properties);
> >> >
> >> > private:
> >> > diff --git a/gcc/passes.c b/gcc/passes.c
> >> > index 19e5869..3b28dc9 100644
> >> > --- a/gcc/passes.c
> >> > +++ b/gcc/passes.c
> >> > @@ -112,7 +112,7 @@ opt_pass::gate ()
> >> > unsigned int
> >> > opt_pass::execute ()
> >> > {
> >> > - return 0;
> >> > + return TODO_absolutely_nothing;
> >> > }
> >> >
> >> > opt_pass::opt_pass (const pass_data &data, context *ctxt)
> >> > @@ -701,33 +701,21 @@ pass_manager::register_one_dump_file (struct opt_pass *pass)
> >> >
> >> > /* Recursive worker function for register_dump_files. */
> >> >
> >> > -int
> >> > +void
> >> > pass_manager::
> >> > -register_dump_files_1 (struct opt_pass *pass, int properties)
> >> > +register_dump_files_1 (struct opt_pass *pass)
> >> > {
> >> > do
> >> > {
> >> > - int new_properties = (properties | pass->properties_provided)
> >> > - & ~pass->properties_destroyed;
> >> > -
> >> > if (pass->name && pass->name[0] != '*')
> >> > register_one_dump_file (pass);
> >> >
> >> > if (pass->sub)
> >> > - new_properties = register_dump_files_1 (pass->sub, new_properties);
> >> > -
> >> > - /* If we have a gate, combine the properties that we could have with
> >> > - and without the pass being examined. */
> >> > - if (pass->has_gate)
> >> > - properties &= new_properties;
> >> > - else
> >> > - properties = new_properties;
> >> > + register_dump_files_1 (pass->sub);
> >> >
> >> > pass = pass->next;
> >> > }
> >> > while (pass);
> >> > -
> >> > - return properties;
> >> > }
> >> >
> >> > /* Register the dump files for the pass_manager starting at PASS.
> >> > @@ -739,7 +727,7 @@ pass_manager::
> >> > register_dump_files (struct opt_pass *pass,int properties)
> >> > {
> >> > pass->properties_required |= properties;
> >> > - register_dump_files_1 (pass, properties);
> >> > + register_dump_files_1 (pass);
> >> > }
> >> >
> >> > struct pass_registry
> >> > @@ -847,7 +835,7 @@ dump_one_pass (struct opt_pass *pass, int pass_indent)
> >> > const char *pn;
> >> > bool is_on, is_really_on;
> >> >
> >> > - is_on = pass->has_gate ? pass->gate () : true;
> >> > + is_on = pass->gate ();
> >> > is_really_on = override_gate_status (pass, current_function_decl, is_on);
> >> >
> >> > if (pass->static_pass_number <= 0)
> >> > @@ -2002,7 +1990,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass)
> >> >
> >> > /* Execute all of the IPA_PASSes in the list. */
> >> > if (ipa_pass->type == IPA_PASS
> >> > - && ((!pass->has_gate) || pass->gate ())
> >> > + && pass->gate ()
> >> > && ipa_pass->generate_summary)
> >> > {
> >> > pass_init_dump_file (pass);
> >> > @@ -2153,7 +2141,7 @@ execute_one_pass (struct opt_pass *pass)
> >> >
> >> > /* Check whether gate check should be avoided.
> >> > User controls the value of the gate through the parameter "gate_status". */
> >> > - gate_status = pass->has_gate ? pass->gate () : true;
> >> > + gate_status = pass->gate ();
> >> > gate_status = override_gate_status (pass, current_function_decl, gate_status);
> >> >
> >> > /* Override gate with plugin. */
> >> > @@ -2210,11 +2198,11 @@ execute_one_pass (struct opt_pass *pass)
> >> > timevar_push (pass->tv_id);
> >> >
> >> > /* Do it! */
> >> > - if (pass->has_execute)
> >> > - {
> >> > - todo_after = pass->execute ();
> >> > - do_per_function (clear_last_verified, NULL);
> >> > - }
> >> > + todo_after = pass->execute ();
> >> > + if (todo_after != TODO_absolutely_nothing)
> >> > + do_per_function (clear_last_verified, NULL);
> >> > + else
> >> > + todo_after &= ~TODO_absolutely_nothing;
> >> >
> >> > /* Stop timevar. */
> >> > if (pass->tv_id != TV_NONE)
> >> > @@ -2286,7 +2274,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state)
> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> >> > if (pass->type == IPA_PASS
> >> > && ipa_pass->write_summary
> >> > - && ((!pass->has_gate) || pass->gate ()))
> >> > + && pass->gate ())
> >> > {
> >> > /* If a timevar is present, start it. */
> >> > if (pass->tv_id)
> >> > @@ -2402,7 +2390,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s
> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> >> > if (pass->type == IPA_PASS
> >> > && ipa_pass->write_optimization_summary
> >> > - && ((!pass->has_gate) || pass->gate ()))
> >> > + && pass->gate ())
> >> > {
> >> > /* If a timevar is present, start it. */
> >> > if (pass->tv_id)
> >> > @@ -2479,7 +2467,7 @@ ipa_read_summaries_1 (struct opt_pass *pass)
> >> > gcc_assert (!cfun);
> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> >> >
> >> > - if ((!pass->has_gate) || pass->gate ())
> >> > + if (pass->gate ())
> >> > {
> >> > if (pass->type == IPA_PASS && ipa_pass->read_summary)
> >> > {
> >> > @@ -2530,7 +2518,7 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass)
> >> > gcc_assert (!cfun);
> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> >> >
> >> > - if ((!pass->has_gate) || pass->gate ())
> >> > + if (pass->gate ())
> >> > {
> >> > if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary)
> >> > {
> >> > @@ -2608,7 +2596,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass,
> >> > {
> >> > /* Execute all of the IPA_PASSes in the list. */
> >> > if (pass->type == IPA_PASS
> >> > - && ((!pass->has_gate) || pass->gate ()))
> >> > + && pass->gate ())
> >> > {
> >> > struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass;
> >> >
> >> > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
> >> > index 9efee1e..bed639e 100644
> >> > --- a/gcc/tree-pass.h
> >> > +++ b/gcc/tree-pass.h
> >> > @@ -49,11 +49,11 @@ struct pass_data
> >> >
> >> > /* If true, this pass has its own implementation of the opt_pass::gate
> >> > method. */
> >> > - bool has_gate;
> >> > + bool useless_has_gate;
> >> >
> >> > /* If true, this pass has its own implementation of the opt_pass::execute
> >> > method. */
> >> > - bool has_execute;
> >> > + bool useless_has_execute;
> >> >
> >> > /* The timevar id associated with this pass. */
> >> > /* ??? Ideally would be dynamically assigned. */
> >> > @@ -299,6 +299,10 @@ protected:
> >> > /* Rebuild the callgraph edges. */
> >> > #define TODO_rebuild_cgraph_edges (1 << 22)
> >> >
> >> > +/* Should only be used by opt_pass::execute to tell the pass manager the pass
> >> > + did absolutely nothing. */
> >> > +#define TODO_absolutely_nothing 1 << 23
> >> > +
> >> > /* Internally used in execute_function_todo(). */
> >> > #define TODO_update_ssa_any \
> >> > (TODO_update_ssa \
> >> > --
> >> > 1.8.4.2
> >> >
[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 1459 bytes --]
diff --git a/gcc/passes.c b/gcc/passes.c
index c008a7b..038bca7 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -112,6 +112,9 @@ opt_pass::gate ()
unsigned int
opt_pass::execute ()
{
+ if (sub)
+ execute_pass_list (sub);
+
return TODO_absolutely_nothing;
}
@@ -2240,8 +2243,7 @@ execute_pass_list (struct opt_pass *pass)
{
gcc_assert (pass->type == GIMPLE_PASS
|| pass->type == RTL_PASS);
- if (execute_one_pass (pass) && pass->sub)
- execute_pass_list (pass->sub);
+ execute_one_pass (pass);
pass = pass->next;
}
while (pass);
@@ -2552,21 +2554,7 @@ execute_ipa_pass_list (struct opt_pass *pass)
gcc_assert (!current_function_decl);
gcc_assert (!cfun);
gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
- if (execute_one_pass (pass) && pass->sub)
- {
- if (pass->sub->type == GIMPLE_PASS)
- {
- invoke_plugin_callbacks (PLUGIN_EARLY_GIMPLE_PASSES_START, NULL);
- do_per_function_toporder ((void (*)(void *))execute_pass_list,
- pass->sub);
- invoke_plugin_callbacks (PLUGIN_EARLY_GIMPLE_PASSES_END, NULL);
- }
- else if (pass->sub->type == SIMPLE_IPA_PASS
- || pass->sub->type == IPA_PASS)
- execute_ipa_pass_list (pass->sub);
- else
- gcc_unreachable ();
- }
+ execute_one_pass (pass);
gcc_assert (!current_function_decl);
cgraph_process_new_functions ();
pass = pass->next;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] make has_gate and has_execute useless
2013-11-08 9:44 ` Richard Biener
2013-11-08 15:42 ` Trevor Saunders
2013-11-11 3:05 ` Trevor Saunders
@ 2013-11-11 18:31 ` Paolo Bonzini
2 siblings, 0 replies; 12+ messages in thread
From: Paolo Bonzini @ 2013-11-11 18:31 UTC (permalink / raw)
To: Richard Biener; +Cc: tsaunders, GCC Patches
Il 08/11/2013 10:37, Richard Biener ha scritto:
> /* If we have a gate, combine the properties that we could have with
> and without the pass being examined. */
> if (pass->has_gate)
> properties &= new_properties;
> else
> properties = new_properties;
>
> which I don't understand (and you just removed all properties handling there).
The properties argument to register_dump_files_1 is indeed dead code; it
is never used except to compute new_properties which is also dead
(because it is simply the argument in a recursive call).
I seem to recall it was needed back when pass->type didn't exist; it
used PROP_rtl to find whether a pass was tree or gimple, or something
like that. The above test and "&=" were needed for this to work at all
optimization levels, but I don't recall the details and as said above
it's all dead anyway.
The argument to register_dump_files is used. To remove it, one could
change all_*_passes from a pointer to a "dummy" pass, and set
properties_required in the initializer for the dummy pass.
Paolo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] make has_gate and has_execute useless
2013-11-11 17:09 ` Trevor Saunders
@ 2013-11-15 13:10 ` Richard Biener
2013-11-15 13:24 ` Trevor Saunders
0 siblings, 1 reply; 12+ messages in thread
From: Richard Biener @ 2013-11-15 13:10 UTC (permalink / raw)
To: Trevor Saunders; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 13865 bytes --]
On Mon, Nov 11, 2013 at 5:13 PM, Trevor Saunders <tsaunders@mozilla.com> wrote:
> On Mon, Nov 11, 2013 at 12:58:36PM +0100, Richard Biener wrote:
>> On Mon, Nov 11, 2013 at 1:39 AM, Trevor Saunders <tsaunders@mozilla.com> wrote:
>> > On Fri, Nov 08, 2013 at 10:37:00AM +0100, Richard Biener wrote:
>> >> On Thu, Nov 7, 2013 at 5:00 PM, <tsaunders@mozilla.com> wrote:
>> >> > From: Trevor Saunders <tsaunders@mozilla.com>
>> >> >
>> >> > Hi,
>> >> >
>> >> > This is the result of seeing what it would take to get rid of the has_gate and
>> >> > has_execute flags on pass_data. It turns out not much, but I wanted
>> >> > confirmation this part is ok before I go deal with all the places that
>> >> > initialize the fields.
>> >> >
>> >> > I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite
>> >> > regressions (ignoring the silk stuff because the full paths in its test names
>> >> > break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok?
>> >>
>> >> The has_gate flag is easy to remove (without a TODO_ hack), right?
>> >> No gate simply means that the gate returns always true. The only
>> >> weird thing is
>> >>
>> >> /* If we have a gate, combine the properties that we could have with
>> >> and without the pass being examined. */
>> >> if (pass->has_gate)
>> >> properties &= new_properties;
>> >> else
>> >> properties = new_properties;
>> >>
>> >> which I don't understand (and you just removed all properties handling there).
>> >>
>> >> So can you split out removing has_gate? This part is obviously ok.
>> >>
>> >> Then, for ->execute () I'd have refactored the code to make
>> >> ->sub passes explicitely executed by the default ->execute ()
>> >> implementation only. That is, passes without overriding execute
>> >> are containers only. Can you quickly check whether that would
>> >> work out?
>> >
>> > Ok, I've now given this a shot and wasn't terribly successful, if I just
>> > change execute_pass_list and execute_ipa_pass_list to handle container
>> > passes executing their sub passes I get the following ICE
>> >
>> > ./gt-passes.h:77:2: internal compiler error: Segmentation fault
>> > };
>> > ^
>> > 0xd43d96 crash_signal
>> > /src/gcc/gcc/toplev.c:334
>> > 0xd901a9 ssa_default_def(function*, tree_node*)
>> > /src/gcc/gcc/tree-dfa.c:318
>> > 0xb56d77 ipa_analyze_params_uses
>> > /src/gcc/gcc/ipa-prop.c:2094
>> > 0xb57275 ipa_analyze_node(cgraph_node*)
>> > /src/gcc/gcc/ipa-prop.c:2179
>> > 0x13e2b6d ipcp_generate_summary
>> > /src/gcc/gcc/ipa-cp.c:3615
>> > 0xc55a2a
>> > execute_ipa_summary_passes(ipa_opt_pass_d*)
>> > /src/gcc/gcc/passes.c:1991
>> > 0x943341 ipa_passes
>> > /src/gcc/gcc/cgraphunit.c:2011
>> > 0x943675
>> > compile()
>> > /src/gcc/gcc/cgraphunit.c:2118
>> >
>> > now
>> > Which is because fn->gimple_df is null. I expect that's because pass
>> > ordering changed somehow, but I'm not sure off hand how or ifthat's
>> > something worth figuring out right now.
>>
>> Yeah, some of the walking doesn't seem to work ... probably a
>> pass with sub-passes already has an execute member function ;)
>
> Sounds like a reasonable guess.
>
>> > Another issue I realized is that doing this will change the order of
>> > plugin events from
>> > start container pass a
>> > end container pass a
>> > start contained pass b
>> > end contained pass b
>> > ...
>> >
>> > to
>> >
>> > start container pass a
>> > start contained pass b
>> > end contained pass b
>> > end container pass a
>> >
>> > Arguably that's an improvement, but I'm not sure if changing that APi
>> > like that is acceptable.
>>
>> Indeed an improvement. Can you attach your patch?
>
> ok, done
It's pass_early_local_passes which has sub-passes and adjusts
cgraph state in its execute.
The following works for me (not really tested of course).
Richard.
> thanks
>
> Trev
>
>>
>> Thanks,
>> Richard.
>>
>> > Trev
>> >
>> >>
>> >> Thanks,
>> >> Richard.
>> >>
>> >> > Trev
>> >> >
>> >> > 2013-11-06 Trevor Saunders <tsaunders@mozilla.com>
>> >> >
>> >> > * pass_manager.h (pass_manager): Adjust.
>> >> > * passes.c (opt_pass::execute): Tell the pass manager it doesn't need
>> >> > to do anything for this pass.
>> >> > (pass_manager::register_dump_files_1): Don't uselessly deal with
>> >> > properties of passes.
>> >> > (pass_manager::register_dump_files): Adjust.
>> >> > (dump_one_pass): Just call pass->gate ().
>> >> > (execute_ipa_summary_passes): Likewise.
>> >> > (execute_one_pass): Don't check pass->has_execute flag.
>> >> > (ipa_write_summaries_2): Don't check pass->has_gate flag.
>> >> > (ipa_write_optimization_summaries_1): Likewise.
>> >> > (ipa_read_summaries_1): Likewise.
>> >> > (ipa_read_optimization_summaries_1): Likewise.
>> >> > (execute_ipa_stmt_fixups): Likewise.
>> >> > * tree-pass.h (pass_data): Rename has_gate to useless_has_gate and
>> >> > has_execute to useless_has_execute to be sure they're unused.
>> >> > (TODO_absolutely_nothing): New constant.
>> >> >
>> >> >
>> >> > diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
>> >> > index 77d78eb..3bc0a99 100644
>> >> > --- a/gcc/pass_manager.h
>> >> > +++ b/gcc/pass_manager.h
>> >> > @@ -93,7 +93,7 @@ public:
>> >> >
>> >> > private:
>> >> > void set_pass_for_id (int id, opt_pass *pass);
>> >> > - int register_dump_files_1 (struct opt_pass *pass, int properties);
>> >> > + void register_dump_files_1 (struct opt_pass *pass);
>> >> > void register_dump_files (struct opt_pass *pass, int properties);
>> >> >
>> >> > private:
>> >> > diff --git a/gcc/passes.c b/gcc/passes.c
>> >> > index 19e5869..3b28dc9 100644
>> >> > --- a/gcc/passes.c
>> >> > +++ b/gcc/passes.c
>> >> > @@ -112,7 +112,7 @@ opt_pass::gate ()
>> >> > unsigned int
>> >> > opt_pass::execute ()
>> >> > {
>> >> > - return 0;
>> >> > + return TODO_absolutely_nothing;
>> >> > }
>> >> >
>> >> > opt_pass::opt_pass (const pass_data &data, context *ctxt)
>> >> > @@ -701,33 +701,21 @@ pass_manager::register_one_dump_file (struct opt_pass *pass)
>> >> >
>> >> > /* Recursive worker function for register_dump_files. */
>> >> >
>> >> > -int
>> >> > +void
>> >> > pass_manager::
>> >> > -register_dump_files_1 (struct opt_pass *pass, int properties)
>> >> > +register_dump_files_1 (struct opt_pass *pass)
>> >> > {
>> >> > do
>> >> > {
>> >> > - int new_properties = (properties | pass->properties_provided)
>> >> > - & ~pass->properties_destroyed;
>> >> > -
>> >> > if (pass->name && pass->name[0] != '*')
>> >> > register_one_dump_file (pass);
>> >> >
>> >> > if (pass->sub)
>> >> > - new_properties = register_dump_files_1 (pass->sub, new_properties);
>> >> > -
>> >> > - /* If we have a gate, combine the properties that we could have with
>> >> > - and without the pass being examined. */
>> >> > - if (pass->has_gate)
>> >> > - properties &= new_properties;
>> >> > - else
>> >> > - properties = new_properties;
>> >> > + register_dump_files_1 (pass->sub);
>> >> >
>> >> > pass = pass->next;
>> >> > }
>> >> > while (pass);
>> >> > -
>> >> > - return properties;
>> >> > }
>> >> >
>> >> > /* Register the dump files for the pass_manager starting at PASS.
>> >> > @@ -739,7 +727,7 @@ pass_manager::
>> >> > register_dump_files (struct opt_pass *pass,int properties)
>> >> > {
>> >> > pass->properties_required |= properties;
>> >> > - register_dump_files_1 (pass, properties);
>> >> > + register_dump_files_1 (pass);
>> >> > }
>> >> >
>> >> > struct pass_registry
>> >> > @@ -847,7 +835,7 @@ dump_one_pass (struct opt_pass *pass, int pass_indent)
>> >> > const char *pn;
>> >> > bool is_on, is_really_on;
>> >> >
>> >> > - is_on = pass->has_gate ? pass->gate () : true;
>> >> > + is_on = pass->gate ();
>> >> > is_really_on = override_gate_status (pass, current_function_decl, is_on);
>> >> >
>> >> > if (pass->static_pass_number <= 0)
>> >> > @@ -2002,7 +1990,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass)
>> >> >
>> >> > /* Execute all of the IPA_PASSes in the list. */
>> >> > if (ipa_pass->type == IPA_PASS
>> >> > - && ((!pass->has_gate) || pass->gate ())
>> >> > + && pass->gate ()
>> >> > && ipa_pass->generate_summary)
>> >> > {
>> >> > pass_init_dump_file (pass);
>> >> > @@ -2153,7 +2141,7 @@ execute_one_pass (struct opt_pass *pass)
>> >> >
>> >> > /* Check whether gate check should be avoided.
>> >> > User controls the value of the gate through the parameter "gate_status". */
>> >> > - gate_status = pass->has_gate ? pass->gate () : true;
>> >> > + gate_status = pass->gate ();
>> >> > gate_status = override_gate_status (pass, current_function_decl, gate_status);
>> >> >
>> >> > /* Override gate with plugin. */
>> >> > @@ -2210,11 +2198,11 @@ execute_one_pass (struct opt_pass *pass)
>> >> > timevar_push (pass->tv_id);
>> >> >
>> >> > /* Do it! */
>> >> > - if (pass->has_execute)
>> >> > - {
>> >> > - todo_after = pass->execute ();
>> >> > - do_per_function (clear_last_verified, NULL);
>> >> > - }
>> >> > + todo_after = pass->execute ();
>> >> > + if (todo_after != TODO_absolutely_nothing)
>> >> > + do_per_function (clear_last_verified, NULL);
>> >> > + else
>> >> > + todo_after &= ~TODO_absolutely_nothing;
>> >> >
>> >> > /* Stop timevar. */
>> >> > if (pass->tv_id != TV_NONE)
>> >> > @@ -2286,7 +2274,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state)
>> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>> >> > if (pass->type == IPA_PASS
>> >> > && ipa_pass->write_summary
>> >> > - && ((!pass->has_gate) || pass->gate ()))
>> >> > + && pass->gate ())
>> >> > {
>> >> > /* If a timevar is present, start it. */
>> >> > if (pass->tv_id)
>> >> > @@ -2402,7 +2390,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s
>> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>> >> > if (pass->type == IPA_PASS
>> >> > && ipa_pass->write_optimization_summary
>> >> > - && ((!pass->has_gate) || pass->gate ()))
>> >> > + && pass->gate ())
>> >> > {
>> >> > /* If a timevar is present, start it. */
>> >> > if (pass->tv_id)
>> >> > @@ -2479,7 +2467,7 @@ ipa_read_summaries_1 (struct opt_pass *pass)
>> >> > gcc_assert (!cfun);
>> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>> >> >
>> >> > - if ((!pass->has_gate) || pass->gate ())
>> >> > + if (pass->gate ())
>> >> > {
>> >> > if (pass->type == IPA_PASS && ipa_pass->read_summary)
>> >> > {
>> >> > @@ -2530,7 +2518,7 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass)
>> >> > gcc_assert (!cfun);
>> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>> >> >
>> >> > - if ((!pass->has_gate) || pass->gate ())
>> >> > + if (pass->gate ())
>> >> > {
>> >> > if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary)
>> >> > {
>> >> > @@ -2608,7 +2596,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass,
>> >> > {
>> >> > /* Execute all of the IPA_PASSes in the list. */
>> >> > if (pass->type == IPA_PASS
>> >> > - && ((!pass->has_gate) || pass->gate ()))
>> >> > + && pass->gate ())
>> >> > {
>> >> > struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass;
>> >> >
>> >> > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
>> >> > index 9efee1e..bed639e 100644
>> >> > --- a/gcc/tree-pass.h
>> >> > +++ b/gcc/tree-pass.h
>> >> > @@ -49,11 +49,11 @@ struct pass_data
>> >> >
>> >> > /* If true, this pass has its own implementation of the opt_pass::gate
>> >> > method. */
>> >> > - bool has_gate;
>> >> > + bool useless_has_gate;
>> >> >
>> >> > /* If true, this pass has its own implementation of the opt_pass::execute
>> >> > method. */
>> >> > - bool has_execute;
>> >> > + bool useless_has_execute;
>> >> >
>> >> > /* The timevar id associated with this pass. */
>> >> > /* ??? Ideally would be dynamically assigned. */
>> >> > @@ -299,6 +299,10 @@ protected:
>> >> > /* Rebuild the callgraph edges. */
>> >> > #define TODO_rebuild_cgraph_edges (1 << 22)
>> >> >
>> >> > +/* Should only be used by opt_pass::execute to tell the pass manager the pass
>> >> > + did absolutely nothing. */
>> >> > +#define TODO_absolutely_nothing 1 << 23
>> >> > +
>> >> > /* Internally used in execute_function_todo(). */
>> >> > #define TODO_update_ssa_any \
>> >> > (TODO_update_ssa \
>> >> > --
>> >> > 1.8.4.2
>> >> >
[-- Attachment #2: p2 --]
[-- Type: application/octet-stream, Size: 3746 bytes --]
Index: gcc/passes.c
===================================================================
--- gcc/passes.c (revision 204787)
+++ gcc/passes.c (working copy)
@@ -112,6 +112,13 @@ opt_pass::gate ()
unsigned int
opt_pass::execute ()
{
+ if (sub)
+ {
+ execute_pass_list (sub);
+ /* Restore current_pass, clobbered by the above, to make TODO
+ processing work. */
+ current_pass = this;
+ }
return 0;
}
@@ -316,20 +323,6 @@ finish_optimization_passes (void)
timevar_pop (TV_DUMP);
}
-static unsigned int
-execute_all_early_local_passes (void)
-{
- /* Once this pass (and its sub-passes) are complete, all functions
- will be in SSA form. Technically this state change is happening
- a tad early, since the sub-passes have not yet run, but since
- none of the sub-passes are IPA passes and do not create new
- functions, this is ok. We're setting this value for the benefit
- of IPA passes that follow. */
- if (cgraph_state < CGRAPH_STATE_IPA_SSA)
- cgraph_state = CGRAPH_STATE_IPA_SSA;
- return 0;
-}
-
/* Gate: execute, or not, all of the non-trivial optimizations. */
static bool
@@ -365,10 +358,32 @@ public:
/* opt_pass methods: */
bool gate () { return gate_all_early_local_passes (); }
- unsigned int execute () { return execute_all_early_local_passes (); }
+ unsigned int execute ();
}; // class pass_early_local_passes
+unsigned int
+pass_early_local_passes::execute()
+{
+ invoke_plugin_callbacks (PLUGIN_EARLY_GIMPLE_PASSES_START, NULL);
+ do_per_function_toporder ((void (*)(void *))execute_pass_list, sub);
+ invoke_plugin_callbacks (PLUGIN_EARLY_GIMPLE_PASSES_END, NULL);
+
+ /* Restore current_pass, clobbered by the above, to make TODO
+ processing work. */
+ current_pass = this;
+
+ /* Once this pass (and its sub-passes) are complete, all functions
+ will be in SSA form. We're setting this value for the benefit
+ of IPA passes that follow. */
+ if (cgraph_state < CGRAPH_STATE_IPA_SSA)
+ cgraph_state = CGRAPH_STATE_IPA_SSA;
+
+ return 0;
+}
+
+
+
} // anon namespace
simple_ipa_opt_pass *
@@ -1851,6 +1866,10 @@ execute_function_todo (void *data)
verify_flow_info ();
if (current_loops && loops_state_satisfies_p (LOOP_CLOSED_SSA))
verify_loop_closed_ssa (false);
+ else if (current_loops
+ && !loops_state_satisfies_p (LOOPS_NEED_FIXUP)
+ && (in_gimple_form || dom_info_available_p (CDI_DOMINATORS)))
+ verify_loop_structure ();
if (flags & TODO_verify_rtl_sharing)
verify_rtl_sharing ();
#endif
@@ -2267,8 +2286,7 @@ execute_pass_list (struct opt_pass *pass
{
gcc_assert (pass->type == GIMPLE_PASS
|| pass->type == RTL_PASS);
- if (execute_one_pass (pass) && pass->sub)
- execute_pass_list (pass->sub);
+ execute_one_pass (pass);
pass = pass->next;
}
while (pass);
@@ -2579,21 +2597,7 @@ execute_ipa_pass_list (struct opt_pass *
gcc_assert (!current_function_decl);
gcc_assert (!cfun);
gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
- if (execute_one_pass (pass) && pass->sub)
- {
- if (pass->sub->type == GIMPLE_PASS)
- {
- invoke_plugin_callbacks (PLUGIN_EARLY_GIMPLE_PASSES_START, NULL);
- do_per_function_toporder ((void (*)(void *))execute_pass_list,
- pass->sub);
- invoke_plugin_callbacks (PLUGIN_EARLY_GIMPLE_PASSES_END, NULL);
- }
- else if (pass->sub->type == SIMPLE_IPA_PASS
- || pass->sub->type == IPA_PASS)
- execute_ipa_pass_list (pass->sub);
- else
- gcc_unreachable ();
- }
+ execute_one_pass (pass);
gcc_assert (!current_function_decl);
cgraph_process_new_functions ();
pass = pass->next;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] make has_gate and has_execute useless
2013-11-15 13:10 ` Richard Biener
@ 2013-11-15 13:24 ` Trevor Saunders
2013-11-15 13:26 ` Richard Biener
0 siblings, 1 reply; 12+ messages in thread
From: Trevor Saunders @ 2013-11-15 13:24 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
On Fri, Nov 15, 2013 at 12:54:05PM +0100, Richard Biener wrote:
> On Mon, Nov 11, 2013 at 5:13 PM, Trevor Saunders <tsaunders@mozilla.com> wrote:
> > On Mon, Nov 11, 2013 at 12:58:36PM +0100, Richard Biener wrote:
> >> On Mon, Nov 11, 2013 at 1:39 AM, Trevor Saunders <tsaunders@mozilla.com> wrote:
> >> > On Fri, Nov 08, 2013 at 10:37:00AM +0100, Richard Biener wrote:
> >> >> On Thu, Nov 7, 2013 at 5:00 PM, <tsaunders@mozilla.com> wrote:
> >> >> > From: Trevor Saunders <tsaunders@mozilla.com>
> >> >> >
> >> >> > Hi,
> >> >> >
> >> >> > This is the result of seeing what it would take to get rid of the has_gate and
> >> >> > has_execute flags on pass_data. It turns out not much, but I wanted
> >> >> > confirmation this part is ok before I go deal with all the places that
> >> >> > initialize the fields.
> >> >> >
> >> >> > I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite
> >> >> > regressions (ignoring the silk stuff because the full paths in its test names
> >> >> > break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok?
> >> >>
> >> >> The has_gate flag is easy to remove (without a TODO_ hack), right?
> >> >> No gate simply means that the gate returns always true. The only
> >> >> weird thing is
> >> >>
> >> >> /* If we have a gate, combine the properties that we could have with
> >> >> and without the pass being examined. */
> >> >> if (pass->has_gate)
> >> >> properties &= new_properties;
> >> >> else
> >> >> properties = new_properties;
> >> >>
> >> >> which I don't understand (and you just removed all properties handling there).
> >> >>
> >> >> So can you split out removing has_gate? This part is obviously ok.
> >> >>
> >> >> Then, for ->execute () I'd have refactored the code to make
> >> >> ->sub passes explicitely executed by the default ->execute ()
> >> >> implementation only. That is, passes without overriding execute
> >> >> are containers only. Can you quickly check whether that would
> >> >> work out?
> >> >
> >> > Ok, I've now given this a shot and wasn't terribly successful, if I just
> >> > change execute_pass_list and execute_ipa_pass_list to handle container
> >> > passes executing their sub passes I get the following ICE
> >> >
> >> > ./gt-passes.h:77:2: internal compiler error: Segmentation fault
> >> > };
> >> > ^
> >> > 0xd43d96 crash_signal
> >> > /src/gcc/gcc/toplev.c:334
> >> > 0xd901a9 ssa_default_def(function*, tree_node*)
> >> > /src/gcc/gcc/tree-dfa.c:318
> >> > 0xb56d77 ipa_analyze_params_uses
> >> > /src/gcc/gcc/ipa-prop.c:2094
> >> > 0xb57275 ipa_analyze_node(cgraph_node*)
> >> > /src/gcc/gcc/ipa-prop.c:2179
> >> > 0x13e2b6d ipcp_generate_summary
> >> > /src/gcc/gcc/ipa-cp.c:3615
> >> > 0xc55a2a
> >> > execute_ipa_summary_passes(ipa_opt_pass_d*)
> >> > /src/gcc/gcc/passes.c:1991
> >> > 0x943341 ipa_passes
> >> > /src/gcc/gcc/cgraphunit.c:2011
> >> > 0x943675
> >> > compile()
> >> > /src/gcc/gcc/cgraphunit.c:2118
> >> >
> >> > now
> >> > Which is because fn->gimple_df is null. I expect that's because pass
> >> > ordering changed somehow, but I'm not sure off hand how or ifthat's
> >> > something worth figuring out right now.
> >>
> >> Yeah, some of the walking doesn't seem to work ... probably a
> >> pass with sub-passes already has an execute member function ;)
> >
> > Sounds like a reasonable guess.
> >
> >> > Another issue I realized is that doing this will change the order of
> >> > plugin events from
> >> > start container pass a
> >> > end container pass a
> >> > start contained pass b
> >> > end contained pass b
> >> > ...
> >> >
> >> > to
> >> >
> >> > start container pass a
> >> > start contained pass b
> >> > end contained pass b
> >> > end container pass a
> >> >
> >> > Arguably that's an improvement, but I'm not sure if changing that APi
> >> > like that is acceptable.
> >>
> >> Indeed an improvement. Can you attach your patch?
> >
> > ok, done
>
> It's pass_early_local_passes which has sub-passes and adjusts
> cgraph state in its execute.
>
> The following works for me (not really tested of course).
ok, thanks a lot for looking, I'll test with the removal of the flags
and repost the full patch.
Trev
>
> Richard.
>
> > thanks
> >
> > Trev
> >
> >>
> >> Thanks,
> >> Richard.
> >>
> >> > Trev
> >> >
> >> >>
> >> >> Thanks,
> >> >> Richard.
> >> >>
> >> >> > Trev
> >> >> >
> >> >> > 2013-11-06 Trevor Saunders <tsaunders@mozilla.com>
> >> >> >
> >> >> > * pass_manager.h (pass_manager): Adjust.
> >> >> > * passes.c (opt_pass::execute): Tell the pass manager it doesn't need
> >> >> > to do anything for this pass.
> >> >> > (pass_manager::register_dump_files_1): Don't uselessly deal with
> >> >> > properties of passes.
> >> >> > (pass_manager::register_dump_files): Adjust.
> >> >> > (dump_one_pass): Just call pass->gate ().
> >> >> > (execute_ipa_summary_passes): Likewise.
> >> >> > (execute_one_pass): Don't check pass->has_execute flag.
> >> >> > (ipa_write_summaries_2): Don't check pass->has_gate flag.
> >> >> > (ipa_write_optimization_summaries_1): Likewise.
> >> >> > (ipa_read_summaries_1): Likewise.
> >> >> > (ipa_read_optimization_summaries_1): Likewise.
> >> >> > (execute_ipa_stmt_fixups): Likewise.
> >> >> > * tree-pass.h (pass_data): Rename has_gate to useless_has_gate and
> >> >> > has_execute to useless_has_execute to be sure they're unused.
> >> >> > (TODO_absolutely_nothing): New constant.
> >> >> >
> >> >> >
> >> >> > diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
> >> >> > index 77d78eb..3bc0a99 100644
> >> >> > --- a/gcc/pass_manager.h
> >> >> > +++ b/gcc/pass_manager.h
> >> >> > @@ -93,7 +93,7 @@ public:
> >> >> >
> >> >> > private:
> >> >> > void set_pass_for_id (int id, opt_pass *pass);
> >> >> > - int register_dump_files_1 (struct opt_pass *pass, int properties);
> >> >> > + void register_dump_files_1 (struct opt_pass *pass);
> >> >> > void register_dump_files (struct opt_pass *pass, int properties);
> >> >> >
> >> >> > private:
> >> >> > diff --git a/gcc/passes.c b/gcc/passes.c
> >> >> > index 19e5869..3b28dc9 100644
> >> >> > --- a/gcc/passes.c
> >> >> > +++ b/gcc/passes.c
> >> >> > @@ -112,7 +112,7 @@ opt_pass::gate ()
> >> >> > unsigned int
> >> >> > opt_pass::execute ()
> >> >> > {
> >> >> > - return 0;
> >> >> > + return TODO_absolutely_nothing;
> >> >> > }
> >> >> >
> >> >> > opt_pass::opt_pass (const pass_data &data, context *ctxt)
> >> >> > @@ -701,33 +701,21 @@ pass_manager::register_one_dump_file (struct opt_pass *pass)
> >> >> >
> >> >> > /* Recursive worker function for register_dump_files. */
> >> >> >
> >> >> > -int
> >> >> > +void
> >> >> > pass_manager::
> >> >> > -register_dump_files_1 (struct opt_pass *pass, int properties)
> >> >> > +register_dump_files_1 (struct opt_pass *pass)
> >> >> > {
> >> >> > do
> >> >> > {
> >> >> > - int new_properties = (properties | pass->properties_provided)
> >> >> > - & ~pass->properties_destroyed;
> >> >> > -
> >> >> > if (pass->name && pass->name[0] != '*')
> >> >> > register_one_dump_file (pass);
> >> >> >
> >> >> > if (pass->sub)
> >> >> > - new_properties = register_dump_files_1 (pass->sub, new_properties);
> >> >> > -
> >> >> > - /* If we have a gate, combine the properties that we could have with
> >> >> > - and without the pass being examined. */
> >> >> > - if (pass->has_gate)
> >> >> > - properties &= new_properties;
> >> >> > - else
> >> >> > - properties = new_properties;
> >> >> > + register_dump_files_1 (pass->sub);
> >> >> >
> >> >> > pass = pass->next;
> >> >> > }
> >> >> > while (pass);
> >> >> > -
> >> >> > - return properties;
> >> >> > }
> >> >> >
> >> >> > /* Register the dump files for the pass_manager starting at PASS.
> >> >> > @@ -739,7 +727,7 @@ pass_manager::
> >> >> > register_dump_files (struct opt_pass *pass,int properties)
> >> >> > {
> >> >> > pass->properties_required |= properties;
> >> >> > - register_dump_files_1 (pass, properties);
> >> >> > + register_dump_files_1 (pass);
> >> >> > }
> >> >> >
> >> >> > struct pass_registry
> >> >> > @@ -847,7 +835,7 @@ dump_one_pass (struct opt_pass *pass, int pass_indent)
> >> >> > const char *pn;
> >> >> > bool is_on, is_really_on;
> >> >> >
> >> >> > - is_on = pass->has_gate ? pass->gate () : true;
> >> >> > + is_on = pass->gate ();
> >> >> > is_really_on = override_gate_status (pass, current_function_decl, is_on);
> >> >> >
> >> >> > if (pass->static_pass_number <= 0)
> >> >> > @@ -2002,7 +1990,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass)
> >> >> >
> >> >> > /* Execute all of the IPA_PASSes in the list. */
> >> >> > if (ipa_pass->type == IPA_PASS
> >> >> > - && ((!pass->has_gate) || pass->gate ())
> >> >> > + && pass->gate ()
> >> >> > && ipa_pass->generate_summary)
> >> >> > {
> >> >> > pass_init_dump_file (pass);
> >> >> > @@ -2153,7 +2141,7 @@ execute_one_pass (struct opt_pass *pass)
> >> >> >
> >> >> > /* Check whether gate check should be avoided.
> >> >> > User controls the value of the gate through the parameter "gate_status". */
> >> >> > - gate_status = pass->has_gate ? pass->gate () : true;
> >> >> > + gate_status = pass->gate ();
> >> >> > gate_status = override_gate_status (pass, current_function_decl, gate_status);
> >> >> >
> >> >> > /* Override gate with plugin. */
> >> >> > @@ -2210,11 +2198,11 @@ execute_one_pass (struct opt_pass *pass)
> >> >> > timevar_push (pass->tv_id);
> >> >> >
> >> >> > /* Do it! */
> >> >> > - if (pass->has_execute)
> >> >> > - {
> >> >> > - todo_after = pass->execute ();
> >> >> > - do_per_function (clear_last_verified, NULL);
> >> >> > - }
> >> >> > + todo_after = pass->execute ();
> >> >> > + if (todo_after != TODO_absolutely_nothing)
> >> >> > + do_per_function (clear_last_verified, NULL);
> >> >> > + else
> >> >> > + todo_after &= ~TODO_absolutely_nothing;
> >> >> >
> >> >> > /* Stop timevar. */
> >> >> > if (pass->tv_id != TV_NONE)
> >> >> > @@ -2286,7 +2274,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state)
> >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> >> >> > if (pass->type == IPA_PASS
> >> >> > && ipa_pass->write_summary
> >> >> > - && ((!pass->has_gate) || pass->gate ()))
> >> >> > + && pass->gate ())
> >> >> > {
> >> >> > /* If a timevar is present, start it. */
> >> >> > if (pass->tv_id)
> >> >> > @@ -2402,7 +2390,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s
> >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> >> >> > if (pass->type == IPA_PASS
> >> >> > && ipa_pass->write_optimization_summary
> >> >> > - && ((!pass->has_gate) || pass->gate ()))
> >> >> > + && pass->gate ())
> >> >> > {
> >> >> > /* If a timevar is present, start it. */
> >> >> > if (pass->tv_id)
> >> >> > @@ -2479,7 +2467,7 @@ ipa_read_summaries_1 (struct opt_pass *pass)
> >> >> > gcc_assert (!cfun);
> >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> >> >> >
> >> >> > - if ((!pass->has_gate) || pass->gate ())
> >> >> > + if (pass->gate ())
> >> >> > {
> >> >> > if (pass->type == IPA_PASS && ipa_pass->read_summary)
> >> >> > {
> >> >> > @@ -2530,7 +2518,7 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass)
> >> >> > gcc_assert (!cfun);
> >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
> >> >> >
> >> >> > - if ((!pass->has_gate) || pass->gate ())
> >> >> > + if (pass->gate ())
> >> >> > {
> >> >> > if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary)
> >> >> > {
> >> >> > @@ -2608,7 +2596,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass,
> >> >> > {
> >> >> > /* Execute all of the IPA_PASSes in the list. */
> >> >> > if (pass->type == IPA_PASS
> >> >> > - && ((!pass->has_gate) || pass->gate ()))
> >> >> > + && pass->gate ())
> >> >> > {
> >> >> > struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass;
> >> >> >
> >> >> > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
> >> >> > index 9efee1e..bed639e 100644
> >> >> > --- a/gcc/tree-pass.h
> >> >> > +++ b/gcc/tree-pass.h
> >> >> > @@ -49,11 +49,11 @@ struct pass_data
> >> >> >
> >> >> > /* If true, this pass has its own implementation of the opt_pass::gate
> >> >> > method. */
> >> >> > - bool has_gate;
> >> >> > + bool useless_has_gate;
> >> >> >
> >> >> > /* If true, this pass has its own implementation of the opt_pass::execute
> >> >> > method. */
> >> >> > - bool has_execute;
> >> >> > + bool useless_has_execute;
> >> >> >
> >> >> > /* The timevar id associated with this pass. */
> >> >> > /* ??? Ideally would be dynamically assigned. */
> >> >> > @@ -299,6 +299,10 @@ protected:
> >> >> > /* Rebuild the callgraph edges. */
> >> >> > #define TODO_rebuild_cgraph_edges (1 << 22)
> >> >> >
> >> >> > +/* Should only be used by opt_pass::execute to tell the pass manager the pass
> >> >> > + did absolutely nothing. */
> >> >> > +#define TODO_absolutely_nothing 1 << 23
> >> >> > +
> >> >> > /* Internally used in execute_function_todo(). */
> >> >> > #define TODO_update_ssa_any \
> >> >> > (TODO_update_ssa \
> >> >> > --
> >> >> > 1.8.4.2
> >> >> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] make has_gate and has_execute useless
2013-11-15 13:24 ` Trevor Saunders
@ 2013-11-15 13:26 ` Richard Biener
0 siblings, 0 replies; 12+ messages in thread
From: Richard Biener @ 2013-11-15 13:26 UTC (permalink / raw)
To: Trevor Saunders; +Cc: GCC Patches
On Fri, Nov 15, 2013 at 1:11 PM, Trevor Saunders <tsaunders@mozilla.com> wrote:
> On Fri, Nov 15, 2013 at 12:54:05PM +0100, Richard Biener wrote:
>> On Mon, Nov 11, 2013 at 5:13 PM, Trevor Saunders <tsaunders@mozilla.com> wrote:
>> > On Mon, Nov 11, 2013 at 12:58:36PM +0100, Richard Biener wrote:
>> >> On Mon, Nov 11, 2013 at 1:39 AM, Trevor Saunders <tsaunders@mozilla.com> wrote:
>> >> > On Fri, Nov 08, 2013 at 10:37:00AM +0100, Richard Biener wrote:
>> >> >> On Thu, Nov 7, 2013 at 5:00 PM, <tsaunders@mozilla.com> wrote:
>> >> >> > From: Trevor Saunders <tsaunders@mozilla.com>
>> >> >> >
>> >> >> > Hi,
>> >> >> >
>> >> >> > This is the result of seeing what it would take to get rid of the has_gate and
>> >> >> > has_execute flags on pass_data. It turns out not much, but I wanted
>> >> >> > confirmation this part is ok before I go deal with all the places that
>> >> >> > initialize the fields.
>> >> >> >
>> >> >> > I bootstrapped this part on x86_64-unknown-linux-gnu and saw no new test suite
>> >> >> > regressions (ignoring the silk stuff because the full paths in its test names
>> >> >> > break my test script for now) Any reason this patch with the actual removal of the flags wouldn't be ok?
>> >> >>
>> >> >> The has_gate flag is easy to remove (without a TODO_ hack), right?
>> >> >> No gate simply means that the gate returns always true. The only
>> >> >> weird thing is
>> >> >>
>> >> >> /* If we have a gate, combine the properties that we could have with
>> >> >> and without the pass being examined. */
>> >> >> if (pass->has_gate)
>> >> >> properties &= new_properties;
>> >> >> else
>> >> >> properties = new_properties;
>> >> >>
>> >> >> which I don't understand (and you just removed all properties handling there).
>> >> >>
>> >> >> So can you split out removing has_gate? This part is obviously ok.
>> >> >>
>> >> >> Then, for ->execute () I'd have refactored the code to make
>> >> >> ->sub passes explicitely executed by the default ->execute ()
>> >> >> implementation only. That is, passes without overriding execute
>> >> >> are containers only. Can you quickly check whether that would
>> >> >> work out?
>> >> >
>> >> > Ok, I've now given this a shot and wasn't terribly successful, if I just
>> >> > change execute_pass_list and execute_ipa_pass_list to handle container
>> >> > passes executing their sub passes I get the following ICE
>> >> >
>> >> > ./gt-passes.h:77:2: internal compiler error: Segmentation fault
>> >> > };
>> >> > ^
>> >> > 0xd43d96 crash_signal
>> >> > /src/gcc/gcc/toplev.c:334
>> >> > 0xd901a9 ssa_default_def(function*, tree_node*)
>> >> > /src/gcc/gcc/tree-dfa.c:318
>> >> > 0xb56d77 ipa_analyze_params_uses
>> >> > /src/gcc/gcc/ipa-prop.c:2094
>> >> > 0xb57275 ipa_analyze_node(cgraph_node*)
>> >> > /src/gcc/gcc/ipa-prop.c:2179
>> >> > 0x13e2b6d ipcp_generate_summary
>> >> > /src/gcc/gcc/ipa-cp.c:3615
>> >> > 0xc55a2a
>> >> > execute_ipa_summary_passes(ipa_opt_pass_d*)
>> >> > /src/gcc/gcc/passes.c:1991
>> >> > 0x943341 ipa_passes
>> >> > /src/gcc/gcc/cgraphunit.c:2011
>> >> > 0x943675
>> >> > compile()
>> >> > /src/gcc/gcc/cgraphunit.c:2118
>> >> >
>> >> > now
>> >> > Which is because fn->gimple_df is null. I expect that's because pass
>> >> > ordering changed somehow, but I'm not sure off hand how or ifthat's
>> >> > something worth figuring out right now.
>> >>
>> >> Yeah, some of the walking doesn't seem to work ... probably a
>> >> pass with sub-passes already has an execute member function ;)
>> >
>> > Sounds like a reasonable guess.
>> >
>> >> > Another issue I realized is that doing this will change the order of
>> >> > plugin events from
>> >> > start container pass a
>> >> > end container pass a
>> >> > start contained pass b
>> >> > end contained pass b
>> >> > ...
>> >> >
>> >> > to
>> >> >
>> >> > start container pass a
>> >> > start contained pass b
>> >> > end contained pass b
>> >> > end container pass a
>> >> >
>> >> > Arguably that's an improvement, but I'm not sure if changing that APi
>> >> > like that is acceptable.
>> >>
>> >> Indeed an improvement. Can you attach your patch?
>> >
>> > ok, done
>>
>> It's pass_early_local_passes which has sub-passes and adjusts
>> cgraph state in its execute.
>>
>> The following works for me (not really tested of course).
>
> ok, thanks a lot for looking, I'll test with the removal of the flags
> and repost the full patch.
There seem to be similar issues elsewhere (from just running
tree-ssa.exp).
Richard.
> Trev
>
>>
>> Richard.
>>
>> > thanks
>> >
>> > Trev
>> >
>> >>
>> >> Thanks,
>> >> Richard.
>> >>
>> >> > Trev
>> >> >
>> >> >>
>> >> >> Thanks,
>> >> >> Richard.
>> >> >>
>> >> >> > Trev
>> >> >> >
>> >> >> > 2013-11-06 Trevor Saunders <tsaunders@mozilla.com>
>> >> >> >
>> >> >> > * pass_manager.h (pass_manager): Adjust.
>> >> >> > * passes.c (opt_pass::execute): Tell the pass manager it doesn't need
>> >> >> > to do anything for this pass.
>> >> >> > (pass_manager::register_dump_files_1): Don't uselessly deal with
>> >> >> > properties of passes.
>> >> >> > (pass_manager::register_dump_files): Adjust.
>> >> >> > (dump_one_pass): Just call pass->gate ().
>> >> >> > (execute_ipa_summary_passes): Likewise.
>> >> >> > (execute_one_pass): Don't check pass->has_execute flag.
>> >> >> > (ipa_write_summaries_2): Don't check pass->has_gate flag.
>> >> >> > (ipa_write_optimization_summaries_1): Likewise.
>> >> >> > (ipa_read_summaries_1): Likewise.
>> >> >> > (ipa_read_optimization_summaries_1): Likewise.
>> >> >> > (execute_ipa_stmt_fixups): Likewise.
>> >> >> > * tree-pass.h (pass_data): Rename has_gate to useless_has_gate and
>> >> >> > has_execute to useless_has_execute to be sure they're unused.
>> >> >> > (TODO_absolutely_nothing): New constant.
>> >> >> >
>> >> >> >
>> >> >> > diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
>> >> >> > index 77d78eb..3bc0a99 100644
>> >> >> > --- a/gcc/pass_manager.h
>> >> >> > +++ b/gcc/pass_manager.h
>> >> >> > @@ -93,7 +93,7 @@ public:
>> >> >> >
>> >> >> > private:
>> >> >> > void set_pass_for_id (int id, opt_pass *pass);
>> >> >> > - int register_dump_files_1 (struct opt_pass *pass, int properties);
>> >> >> > + void register_dump_files_1 (struct opt_pass *pass);
>> >> >> > void register_dump_files (struct opt_pass *pass, int properties);
>> >> >> >
>> >> >> > private:
>> >> >> > diff --git a/gcc/passes.c b/gcc/passes.c
>> >> >> > index 19e5869..3b28dc9 100644
>> >> >> > --- a/gcc/passes.c
>> >> >> > +++ b/gcc/passes.c
>> >> >> > @@ -112,7 +112,7 @@ opt_pass::gate ()
>> >> >> > unsigned int
>> >> >> > opt_pass::execute ()
>> >> >> > {
>> >> >> > - return 0;
>> >> >> > + return TODO_absolutely_nothing;
>> >> >> > }
>> >> >> >
>> >> >> > opt_pass::opt_pass (const pass_data &data, context *ctxt)
>> >> >> > @@ -701,33 +701,21 @@ pass_manager::register_one_dump_file (struct opt_pass *pass)
>> >> >> >
>> >> >> > /* Recursive worker function for register_dump_files. */
>> >> >> >
>> >> >> > -int
>> >> >> > +void
>> >> >> > pass_manager::
>> >> >> > -register_dump_files_1 (struct opt_pass *pass, int properties)
>> >> >> > +register_dump_files_1 (struct opt_pass *pass)
>> >> >> > {
>> >> >> > do
>> >> >> > {
>> >> >> > - int new_properties = (properties | pass->properties_provided)
>> >> >> > - & ~pass->properties_destroyed;
>> >> >> > -
>> >> >> > if (pass->name && pass->name[0] != '*')
>> >> >> > register_one_dump_file (pass);
>> >> >> >
>> >> >> > if (pass->sub)
>> >> >> > - new_properties = register_dump_files_1 (pass->sub, new_properties);
>> >> >> > -
>> >> >> > - /* If we have a gate, combine the properties that we could have with
>> >> >> > - and without the pass being examined. */
>> >> >> > - if (pass->has_gate)
>> >> >> > - properties &= new_properties;
>> >> >> > - else
>> >> >> > - properties = new_properties;
>> >> >> > + register_dump_files_1 (pass->sub);
>> >> >> >
>> >> >> > pass = pass->next;
>> >> >> > }
>> >> >> > while (pass);
>> >> >> > -
>> >> >> > - return properties;
>> >> >> > }
>> >> >> >
>> >> >> > /* Register the dump files for the pass_manager starting at PASS.
>> >> >> > @@ -739,7 +727,7 @@ pass_manager::
>> >> >> > register_dump_files (struct opt_pass *pass,int properties)
>> >> >> > {
>> >> >> > pass->properties_required |= properties;
>> >> >> > - register_dump_files_1 (pass, properties);
>> >> >> > + register_dump_files_1 (pass);
>> >> >> > }
>> >> >> >
>> >> >> > struct pass_registry
>> >> >> > @@ -847,7 +835,7 @@ dump_one_pass (struct opt_pass *pass, int pass_indent)
>> >> >> > const char *pn;
>> >> >> > bool is_on, is_really_on;
>> >> >> >
>> >> >> > - is_on = pass->has_gate ? pass->gate () : true;
>> >> >> > + is_on = pass->gate ();
>> >> >> > is_really_on = override_gate_status (pass, current_function_decl, is_on);
>> >> >> >
>> >> >> > if (pass->static_pass_number <= 0)
>> >> >> > @@ -2002,7 +1990,7 @@ execute_ipa_summary_passes (struct ipa_opt_pass_d *ipa_pass)
>> >> >> >
>> >> >> > /* Execute all of the IPA_PASSes in the list. */
>> >> >> > if (ipa_pass->type == IPA_PASS
>> >> >> > - && ((!pass->has_gate) || pass->gate ())
>> >> >> > + && pass->gate ()
>> >> >> > && ipa_pass->generate_summary)
>> >> >> > {
>> >> >> > pass_init_dump_file (pass);
>> >> >> > @@ -2153,7 +2141,7 @@ execute_one_pass (struct opt_pass *pass)
>> >> >> >
>> >> >> > /* Check whether gate check should be avoided.
>> >> >> > User controls the value of the gate through the parameter "gate_status". */
>> >> >> > - gate_status = pass->has_gate ? pass->gate () : true;
>> >> >> > + gate_status = pass->gate ();
>> >> >> > gate_status = override_gate_status (pass, current_function_decl, gate_status);
>> >> >> >
>> >> >> > /* Override gate with plugin. */
>> >> >> > @@ -2210,11 +2198,11 @@ execute_one_pass (struct opt_pass *pass)
>> >> >> > timevar_push (pass->tv_id);
>> >> >> >
>> >> >> > /* Do it! */
>> >> >> > - if (pass->has_execute)
>> >> >> > - {
>> >> >> > - todo_after = pass->execute ();
>> >> >> > - do_per_function (clear_last_verified, NULL);
>> >> >> > - }
>> >> >> > + todo_after = pass->execute ();
>> >> >> > + if (todo_after != TODO_absolutely_nothing)
>> >> >> > + do_per_function (clear_last_verified, NULL);
>> >> >> > + else
>> >> >> > + todo_after &= ~TODO_absolutely_nothing;
>> >> >> >
>> >> >> > /* Stop timevar. */
>> >> >> > if (pass->tv_id != TV_NONE)
>> >> >> > @@ -2286,7 +2274,7 @@ ipa_write_summaries_2 (struct opt_pass *pass, struct lto_out_decl_state *state)
>> >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>> >> >> > if (pass->type == IPA_PASS
>> >> >> > && ipa_pass->write_summary
>> >> >> > - && ((!pass->has_gate) || pass->gate ()))
>> >> >> > + && pass->gate ())
>> >> >> > {
>> >> >> > /* If a timevar is present, start it. */
>> >> >> > if (pass->tv_id)
>> >> >> > @@ -2402,7 +2390,7 @@ ipa_write_optimization_summaries_1 (struct opt_pass *pass, struct lto_out_decl_s
>> >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>> >> >> > if (pass->type == IPA_PASS
>> >> >> > && ipa_pass->write_optimization_summary
>> >> >> > - && ((!pass->has_gate) || pass->gate ()))
>> >> >> > + && pass->gate ())
>> >> >> > {
>> >> >> > /* If a timevar is present, start it. */
>> >> >> > if (pass->tv_id)
>> >> >> > @@ -2479,7 +2467,7 @@ ipa_read_summaries_1 (struct opt_pass *pass)
>> >> >> > gcc_assert (!cfun);
>> >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>> >> >> >
>> >> >> > - if ((!pass->has_gate) || pass->gate ())
>> >> >> > + if (pass->gate ())
>> >> >> > {
>> >> >> > if (pass->type == IPA_PASS && ipa_pass->read_summary)
>> >> >> > {
>> >> >> > @@ -2530,7 +2518,7 @@ ipa_read_optimization_summaries_1 (struct opt_pass *pass)
>> >> >> > gcc_assert (!cfun);
>> >> >> > gcc_assert (pass->type == SIMPLE_IPA_PASS || pass->type == IPA_PASS);
>> >> >> >
>> >> >> > - if ((!pass->has_gate) || pass->gate ())
>> >> >> > + if (pass->gate ())
>> >> >> > {
>> >> >> > if (pass->type == IPA_PASS && ipa_pass->read_optimization_summary)
>> >> >> > {
>> >> >> > @@ -2608,7 +2596,7 @@ execute_ipa_stmt_fixups (struct opt_pass *pass,
>> >> >> > {
>> >> >> > /* Execute all of the IPA_PASSes in the list. */
>> >> >> > if (pass->type == IPA_PASS
>> >> >> > - && ((!pass->has_gate) || pass->gate ()))
>> >> >> > + && pass->gate ())
>> >> >> > {
>> >> >> > struct ipa_opt_pass_d *ipa_pass = (struct ipa_opt_pass_d *) pass;
>> >> >> >
>> >> >> > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
>> >> >> > index 9efee1e..bed639e 100644
>> >> >> > --- a/gcc/tree-pass.h
>> >> >> > +++ b/gcc/tree-pass.h
>> >> >> > @@ -49,11 +49,11 @@ struct pass_data
>> >> >> >
>> >> >> > /* If true, this pass has its own implementation of the opt_pass::gate
>> >> >> > method. */
>> >> >> > - bool has_gate;
>> >> >> > + bool useless_has_gate;
>> >> >> >
>> >> >> > /* If true, this pass has its own implementation of the opt_pass::execute
>> >> >> > method. */
>> >> >> > - bool has_execute;
>> >> >> > + bool useless_has_execute;
>> >> >> >
>> >> >> > /* The timevar id associated with this pass. */
>> >> >> > /* ??? Ideally would be dynamically assigned. */
>> >> >> > @@ -299,6 +299,10 @@ protected:
>> >> >> > /* Rebuild the callgraph edges. */
>> >> >> > #define TODO_rebuild_cgraph_edges (1 << 22)
>> >> >> >
>> >> >> > +/* Should only be used by opt_pass::execute to tell the pass manager the pass
>> >> >> > + did absolutely nothing. */
>> >> >> > +#define TODO_absolutely_nothing 1 << 23
>> >> >> > +
>> >> >> > /* Internally used in execute_function_todo(). */
>> >> >> > #define TODO_update_ssa_any \
>> >> >> > (TODO_update_ssa \
>> >> >> > --
>> >> >> > 1.8.4.2
>> >> >> >
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-11-15 12:15 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-07 16:40 [PATCH] make has_gate and has_execute useless tsaunders
2013-11-07 17:51 ` Jeff Law
2013-11-07 18:38 ` Trevor Saunders
2013-11-08 9:44 ` Richard Biener
2013-11-08 15:42 ` Trevor Saunders
2013-11-11 3:05 ` Trevor Saunders
2013-11-11 12:16 ` Richard Biener
2013-11-11 17:09 ` Trevor Saunders
2013-11-15 13:10 ` Richard Biener
2013-11-15 13:24 ` Trevor Saunders
2013-11-15 13:26 ` Richard Biener
2013-11-11 18:31 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).