Fix ICE with -ftree-parallelize-loops=2 -fno-ipa-pure-const (PR ipa/83506)

Message ID 20171220192404.GL2353@tucnak
State New
Headers show
Series
  • Fix ICE with -ftree-parallelize-loops=2 -fno-ipa-pure-const (PR ipa/83506)
Related show

Commit Message

Jakub Jelinek Dec. 20, 2017, 7:24 p.m.
Hi!

Prathamesh's r254140 moved the
  if (!flag_wpa)
    ipa_free_fn_summary ();
from the ipa-inline pass to the following ipa-pure-const pass.
That doesn't work well though, because ipa-inline pass has unconditional
gate, so is done always when real IPA passes are performed, but
ipa-pure-const is gated on some flag_* options, so if those options
are disabled, we keep fn summary computed until end of compilation, and
e.g. new function insertion hooks then attempt to update the fnsummary
even when with RTL hooks registered.

Calling ipa_free_fn_summary () in ipa-inline pass if the gate of
the following pass will not be run looks too ugly to me, and while there
is another unconditional ipa pass, moving it there doesn't look too clean to
me either.

We already have a separate pass ipa-free-fn-summary, just use it so far only
during small IPA passes.  I think the cleanest is to add another ipa pass
that will just free it instead of sticking it into unrelated passes which
just happen to be unconditional ATM.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2017-12-20  Jakub Jelinek  <jakub@redhat.com>

	PR ipa/83506
	* ipa-fnsummary.c (pass_data_ipa_free_fn_summary): Use 0 for
	todo_flags_finish.
	(pass_ipa_free_fn_summary): Add small_p private data member,
	initialize to false in the ctor.
	(pass_ipa_free_fn_summary::clone,
	pass_ipa_free_fn_summary::set_pass_param,
	pass_ipa_free_fn_summary::gate): New methods.
	(pass_ipa_free_fn_summary::execute): Return TODO_remove_functions
	| TODO_dump_symtab if small_p.
	* passes.def: Add true parm for the existing pass_ipa_free_fn_summary
	entry and add another instance of the pass with false parm after
	ipa-pure-const.
	* ipa-pure-const.c (pass_ipa_pure_const): Don't call
	ipa_free_fn_summary here.

	* gcc.dg/pr83506.c: New test.
	* gcc.dg/ipa/ctor-empty-1.c: Use -fdump-ipa-free-fnsummary1 instead
	of -fdump-ipa-free-fnsummary and scan in free-fnsummary1 instead of
	free-fnsummary dump.


	Jakub

Comments

Richard Biener Dec. 20, 2017, 7:37 p.m. | #1
On December 20, 2017 8:24:04 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!

>

>Prathamesh's r254140 moved the

>  if (!flag_wpa)

>    ipa_free_fn_summary ();

>from the ipa-inline pass to the following ipa-pure-const pass.

>That doesn't work well though, because ipa-inline pass has

>unconditional

>gate, so is done always when real IPA passes are performed, but

>ipa-pure-const is gated on some flag_* options, so if those options

>are disabled, we keep fn summary computed until end of compilation, and

>e.g. new function insertion hooks then attempt to update the fnsummary

>even when with RTL hooks registered.

>

>Calling ipa_free_fn_summary () in ipa-inline pass if the gate of

>the following pass will not be run looks too ugly to me, and while

>there

>is another unconditional ipa pass, moving it there doesn't look too

>clean to

>me either.

>

>We already have a separate pass ipa-free-fn-summary, just use it so far

>only

>during small IPA passes.  I think the cleanest is to add another ipa

>pass

>that will just free it instead of sticking it into unrelated passes

>which

>just happen to be unconditional ATM.

>

>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?


OK. 

Richard. 

>2017-12-20  Jakub Jelinek  <jakub@redhat.com>

>

>	PR ipa/83506

>	* ipa-fnsummary.c (pass_data_ipa_free_fn_summary): Use 0 for

>	todo_flags_finish.

>	(pass_ipa_free_fn_summary): Add small_p private data member,

>	initialize to false in the ctor.

>	(pass_ipa_free_fn_summary::clone,

>	pass_ipa_free_fn_summary::set_pass_param,

>	pass_ipa_free_fn_summary::gate): New methods.

>	(pass_ipa_free_fn_summary::execute): Return TODO_remove_functions

>	| TODO_dump_symtab if small_p.

>	* passes.def: Add true parm for the existing pass_ipa_free_fn_summary

>	entry and add another instance of the pass with false parm after

>	ipa-pure-const.

>	* ipa-pure-const.c (pass_ipa_pure_const): Don't call

>	ipa_free_fn_summary here.

>

>	* gcc.dg/pr83506.c: New test.

>	* gcc.dg/ipa/ctor-empty-1.c: Use -fdump-ipa-free-fnsummary1 instead

>	of -fdump-ipa-free-fnsummary and scan in free-fnsummary1 instead of

>	free-fnsummary dump.

>

>--- gcc/ipa-fnsummary.c.jj	2017-12-20 11:01:13.000000000 +0100

>+++ gcc/ipa-fnsummary.c	2017-12-20 11:43:40.462288141 +0100

>@@ -3538,26 +3538,36 @@ const pass_data pass_data_ipa_free_fn_su

>   0, /* properties_provided */

>   0, /* properties_destroyed */

>   0, /* todo_flags_start */

>-  /* Early optimizations may make function unreachable.  We can not

>-     remove unreachable functions as part of the ealry opts pass

>because

>-     TODOs are run before subpasses.  Do it here.  */

>-  ( TODO_remove_functions | TODO_dump_symtab ), /* todo_flags_finish

>*/

>+  0, /* todo_flags_finish */

> };

> 

> class pass_ipa_free_fn_summary : public simple_ipa_opt_pass

> {

> public:

>   pass_ipa_free_fn_summary (gcc::context *ctxt)

>-    : simple_ipa_opt_pass (pass_data_ipa_free_fn_summary, ctxt)

>+    : simple_ipa_opt_pass (pass_data_ipa_free_fn_summary, ctxt),

>+      small_p (false)

>   {}

> 

>   /* opt_pass methods: */

>+  opt_pass *clone () { return new pass_ipa_free_fn_summary (m_ctxt); }

>+  void set_pass_param (unsigned int n, bool param)

>+    {

>+      gcc_assert (n == 0);

>+      small_p = param;

>+    }

>+  virtual bool gate (function *) { return small_p || !flag_wpa; }

>   virtual unsigned int execute (function *)

>     {

>       ipa_free_fn_summary ();

>-      return 0;

>+      /* Early optimizations may make function unreachable.  We can

>not

>+	 remove unreachable functions as part of the early opts pass because

>+	 TODOs are run before subpasses.  Do it here.  */

>+      return small_p ? TODO_remove_functions | TODO_dump_symtab : 0;

>     }

> 

>+private:

>+  bool small_p;

> }; // class pass_ipa_free_fn_summary

> 

> } // anon namespace

>--- gcc/passes.def.jj	2017-12-18 14:57:20.000000000 +0100

>+++ gcc/passes.def	2017-12-20 11:31:32.402117369 +0100

>@@ -144,7 +144,7 @@ along with GCC; see the file COPYING3.

>   PUSH_INSERT_PASSES_WITHIN (pass_ipa_tree_profile)

>       NEXT_PASS (pass_feedback_split_functions);

>   POP_INSERT_PASSES ()

>-  NEXT_PASS (pass_ipa_free_fn_summary);

>+  NEXT_PASS (pass_ipa_free_fn_summary, true /* small_p */);

>   NEXT_PASS (pass_ipa_increase_alignment);

>   NEXT_PASS (pass_ipa_tm);

>   NEXT_PASS (pass_ipa_lower_emutls);

>@@ -161,6 +161,7 @@ along with GCC; see the file COPYING3.

>   NEXT_PASS (pass_ipa_fn_summary);

>   NEXT_PASS (pass_ipa_inline);

>   NEXT_PASS (pass_ipa_pure_const);

>+  NEXT_PASS (pass_ipa_free_fn_summary, false /* small_p */);

>   NEXT_PASS (pass_ipa_reference);

> /* This pass needs to be scheduled after any IP code duplication.   */

>   NEXT_PASS (pass_ipa_single_use);

>--- gcc/ipa-pure-const.c.jj	2017-12-04 22:29:11.000000000 +0100

>+++ gcc/ipa-pure-const.c	2017-12-20 11:33:11.905956408 +0100

>@@ -2013,10 +2013,6 @@ execute (function *)

>     if (has_function_state (node))

>       free (get_function_state (node));

>   funct_state_vec.release ();

>-

>-  /* In WPA we use inline summaries for partitioning process.  */

>-  if (!flag_wpa)

>-    ipa_free_fn_summary ();

>   return remove_p ? TODO_remove_functions : 0;

> }

> 

>--- gcc/testsuite/gcc.dg/pr83506.c.jj	2017-12-20 11:39:48.405212526

>+0100

>+++ gcc/testsuite/gcc.dg/pr83506.c	2017-12-20 11:39:18.000000000 +0100

>@@ -0,0 +1,14 @@

>+/* PR ipa/83506 */

>+/* { dg-do compile { target pthread } } */

>+/* { dg-options "-O1 -ftree-parallelize-loops=2 -fno-ipa-pure-const" }

>*/

>+

>+unsigned int

>+foo (unsigned int x, int y)

>+{

>+  while (y < 1)

>+    {

>+      x *= 3;

>+      ++y;

>+    }

>+  return x;

>+}

>--- gcc/testsuite/gcc.dg/ipa/ctor-empty-1.c.jj	2017-05-24

>11:59:01.000000000 +0200

>+++ gcc/testsuite/gcc.dg/ipa/ctor-empty-1.c	2017-12-20

>20:15:27.144761517 +0100

>@@ -1,7 +1,7 @@

> /* { dg-do compile } */

>-/* { dg-options "-O3 -c -fdump-ipa-free-fnsummary"  } */

>+/* { dg-options "-O3 -c -fdump-ipa-free-fnsummary1"  } */

> static __attribute__((constructor))

> void empty_constructor()

> {

> }

>-/* { dg-final { scan-ipa-dump "Reclaiming functions:

>empty_constructor"  "free-fnsummary"  } } */

>+/* { dg-final { scan-ipa-dump "Reclaiming functions:

>empty_constructor"  "free-fnsummary1"  } } */

>

>	Jakub
David Malcolm Dec. 20, 2017, 7:38 p.m. | #2
On Wed, 2017-12-20 at 20:24 +0100, Jakub Jelinek wrote:
> Hi!

> 

> Prathamesh's r254140 moved the

>   if (!flag_wpa)

>     ipa_free_fn_summary ();

> from the ipa-inline pass to the following ipa-pure-const pass.

> That doesn't work well though, because ipa-inline pass has

> unconditional

> gate, so is done always when real IPA passes are performed, but

> ipa-pure-const is gated on some flag_* options, so if those options

> are disabled, we keep fn summary computed until end of compilation,

> and

> e.g. new function insertion hooks then attempt to update the

> fnsummary

> even when with RTL hooks registered.

> 

> Calling ipa_free_fn_summary () in ipa-inline pass if the gate of

> the following pass will not be run looks too ugly to me, and while

> there

> is another unconditional ipa pass, moving it there doesn't look too

> clean to

> me either.

> 

> We already have a separate pass ipa-free-fn-summary, just use it so

> far only

> during small IPA passes.  I think the cleanest is to add another ipa

> pass

> that will just free it instead of sticking it into unrelated passes

> which

> just happen to be unconditional ATM.

> 

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?


Out of interest, did you test this with "jit" enabled?  I mention it
because we ran into issues with ipa fn summary cleanup with jit before,
which required r254458 to fix (PR jit/82826).

Thanks
Dave

> 2017-12-20  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR ipa/83506

> 	* ipa-fnsummary.c (pass_data_ipa_free_fn_summary): Use 0 for

> 	todo_flags_finish.

> 	(pass_ipa_free_fn_summary): Add small_p private data member,

> 	initialize to false in the ctor.

> 	(pass_ipa_free_fn_summary::clone,

> 	pass_ipa_free_fn_summary::set_pass_param,

> 	pass_ipa_free_fn_summary::gate): New methods.

> 	(pass_ipa_free_fn_summary::execute): Return

> TODO_remove_functions

> 	| TODO_dump_symtab if small_p.

> 	* passes.def: Add true parm for the existing

> pass_ipa_free_fn_summary

> 	entry and add another instance of the pass with false parm

> after

> 	ipa-pure-const.

> 	* ipa-pure-const.c (pass_ipa_pure_const): Don't call

> 	ipa_free_fn_summary here.

> 

> 	* gcc.dg/pr83506.c: New test.

> 	* gcc.dg/ipa/ctor-empty-1.c: Use -fdump-ipa-free-fnsummary1

> instead

> 	of -fdump-ipa-free-fnsummary and scan in free-fnsummary1

> instead of

> 	free-fnsummary dump.

> 

> --- gcc/ipa-fnsummary.c.jj	2017-12-20 11:01:13.000000000 +0100

> +++ gcc/ipa-fnsummary.c	2017-12-20 11:43:40.462288141 +0100

> @@ -3538,26 +3538,36 @@ const pass_data pass_data_ipa_free_fn_su

>    0, /* properties_provided */

>    0, /* properties_destroyed */

>    0, /* todo_flags_start */

> -  /* Early optimizations may make function unreachable.  We can not

> -     remove unreachable functions as part of the ealry opts pass

> because

> -     TODOs are run before subpasses.  Do it here.  */

> -  ( TODO_remove_functions | TODO_dump_symtab ), /* todo_flags_finish

> */

> +  0, /* todo_flags_finish */

>  };

>  

>  class pass_ipa_free_fn_summary : public simple_ipa_opt_pass

>  {

>  public:

>    pass_ipa_free_fn_summary (gcc::context *ctxt)

> -    : simple_ipa_opt_pass (pass_data_ipa_free_fn_summary, ctxt)

> +    : simple_ipa_opt_pass (pass_data_ipa_free_fn_summary, ctxt),

> +      small_p (false)

>    {}

>  

>    /* opt_pass methods: */

> +  opt_pass *clone () { return new pass_ipa_free_fn_summary (m_ctxt);

> }

> +  void set_pass_param (unsigned int n, bool param)

> +    {

> +      gcc_assert (n == 0);

> +      small_p = param;

> +    }

> +  virtual bool gate (function *) { return small_p || !flag_wpa; }

>    virtual unsigned int execute (function *)

>      {

>        ipa_free_fn_summary ();

> -      return 0;

> +      /* Early optimizations may make function unreachable.  We can

> not

> +	 remove unreachable functions as part of the early opts pass

> because

> +	 TODOs are run before subpasses.  Do it here.  */

> +      return small_p ? TODO_remove_functions | TODO_dump_symtab : 0;

>      }

>  

> +private:

> +  bool small_p;

>  }; // class pass_ipa_free_fn_summary

>  

>  } // anon namespace

> --- gcc/passes.def.jj	2017-12-18 14:57:20.000000000 +0100

> +++ gcc/passes.def	2017-12-20 11:31:32.402117369 +0100

> @@ -144,7 +144,7 @@ along with GCC; see the file COPYING3.

>    PUSH_INSERT_PASSES_WITHIN (pass_ipa_tree_profile)

>        NEXT_PASS (pass_feedback_split_functions);

>    POP_INSERT_PASSES ()

> -  NEXT_PASS (pass_ipa_free_fn_summary);

> +  NEXT_PASS (pass_ipa_free_fn_summary, true /* small_p */);

>    NEXT_PASS (pass_ipa_increase_alignment);

>    NEXT_PASS (pass_ipa_tm);

>    NEXT_PASS (pass_ipa_lower_emutls);

> @@ -161,6 +161,7 @@ along with GCC; see the file COPYING3.

>    NEXT_PASS (pass_ipa_fn_summary);

>    NEXT_PASS (pass_ipa_inline);

>    NEXT_PASS (pass_ipa_pure_const);

> +  NEXT_PASS (pass_ipa_free_fn_summary, false /* small_p */);

>    NEXT_PASS (pass_ipa_reference);

>    /* This pass needs to be scheduled after any IP code

> duplication.   */

>    NEXT_PASS (pass_ipa_single_use);

> --- gcc/ipa-pure-const.c.jj	2017-12-04 22:29:11.000000000

> +0100

> +++ gcc/ipa-pure-const.c	2017-12-20 11:33:11.905956408 +0100

> @@ -2013,10 +2013,6 @@ execute (function *)

>      if (has_function_state (node))

>        free (get_function_state (node));

>    funct_state_vec.release ();

> -

> -  /* In WPA we use inline summaries for partitioning process.  */

> -  if (!flag_wpa)

> -    ipa_free_fn_summary ();

>    return remove_p ? TODO_remove_functions : 0;

>  }

>  

> --- gcc/testsuite/gcc.dg/pr83506.c.jj	2017-12-20

> 11:39:48.405212526 +0100

> +++ gcc/testsuite/gcc.dg/pr83506.c	2017-12-20

> 11:39:18.000000000 +0100

> @@ -0,0 +1,14 @@

> +/* PR ipa/83506 */

> +/* { dg-do compile { target pthread } } */

> +/* { dg-options "-O1 -ftree-parallelize-loops=2 -fno-ipa-pure-const" 

> } */

> +

> +unsigned int

> +foo (unsigned int x, int y)

> +{

> +  while (y < 1)

> +    {

> +      x *= 3;

> +      ++y;

> +    }

> +  return x;

> +}

> --- gcc/testsuite/gcc.dg/ipa/ctor-empty-1.c.jj	2017-05-24

> 11:59:01.000000000 +0200

> +++ gcc/testsuite/gcc.dg/ipa/ctor-empty-1.c	2017-12-20

> 20:15:27.144761517 +0100

> @@ -1,7 +1,7 @@

>  /* { dg-do compile } */

> -/* { dg-options "-O3 -c -fdump-ipa-free-fnsummary"  } */

> +/* { dg-options "-O3 -c -fdump-ipa-free-fnsummary1"  } */

>  static __attribute__((constructor))

>  void empty_constructor()

>  {

>  }

> -/* { dg-final { scan-ipa-dump "Reclaiming functions:

> empty_constructor"  "free-fnsummary"  } } */

> +/* { dg-final { scan-ipa-dump "Reclaiming functions:

> empty_constructor"  "free-fnsummary1"  } } */

> 

> 	Jakub
Jakub Jelinek Dec. 20, 2017, 7:39 p.m. | #3
On Wed, Dec 20, 2017 at 02:38:13PM -0500, David Malcolm wrote:
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

> 

> Out of interest, did you test this with "jit" enabled?  I mention it

> because we ran into issues with ipa fn summary cleanup with jit before,

> which required r254458 to fix (PR jit/82826).


No, I'm testing jit only when building distro rpms, not in normal bootstraps
(because it requires --enable-host-shared).

	Jakub

Patch

--- gcc/ipa-fnsummary.c.jj	2017-12-20 11:01:13.000000000 +0100
+++ gcc/ipa-fnsummary.c	2017-12-20 11:43:40.462288141 +0100
@@ -3538,26 +3538,36 @@  const pass_data pass_data_ipa_free_fn_su
   0, /* properties_provided */
   0, /* properties_destroyed */
   0, /* todo_flags_start */
-  /* Early optimizations may make function unreachable.  We can not
-     remove unreachable functions as part of the ealry opts pass because
-     TODOs are run before subpasses.  Do it here.  */
-  ( TODO_remove_functions | TODO_dump_symtab ), /* todo_flags_finish */
+  0, /* todo_flags_finish */
 };
 
 class pass_ipa_free_fn_summary : public simple_ipa_opt_pass
 {
 public:
   pass_ipa_free_fn_summary (gcc::context *ctxt)
-    : simple_ipa_opt_pass (pass_data_ipa_free_fn_summary, ctxt)
+    : simple_ipa_opt_pass (pass_data_ipa_free_fn_summary, ctxt),
+      small_p (false)
   {}
 
   /* opt_pass methods: */
+  opt_pass *clone () { return new pass_ipa_free_fn_summary (m_ctxt); }
+  void set_pass_param (unsigned int n, bool param)
+    {
+      gcc_assert (n == 0);
+      small_p = param;
+    }
+  virtual bool gate (function *) { return small_p || !flag_wpa; }
   virtual unsigned int execute (function *)
     {
       ipa_free_fn_summary ();
-      return 0;
+      /* Early optimizations may make function unreachable.  We can not
+	 remove unreachable functions as part of the early opts pass because
+	 TODOs are run before subpasses.  Do it here.  */
+      return small_p ? TODO_remove_functions | TODO_dump_symtab : 0;
     }
 
+private:
+  bool small_p;
 }; // class pass_ipa_free_fn_summary
 
 } // anon namespace
--- gcc/passes.def.jj	2017-12-18 14:57:20.000000000 +0100
+++ gcc/passes.def	2017-12-20 11:31:32.402117369 +0100
@@ -144,7 +144,7 @@  along with GCC; see the file COPYING3.
   PUSH_INSERT_PASSES_WITHIN (pass_ipa_tree_profile)
       NEXT_PASS (pass_feedback_split_functions);
   POP_INSERT_PASSES ()
-  NEXT_PASS (pass_ipa_free_fn_summary);
+  NEXT_PASS (pass_ipa_free_fn_summary, true /* small_p */);
   NEXT_PASS (pass_ipa_increase_alignment);
   NEXT_PASS (pass_ipa_tm);
   NEXT_PASS (pass_ipa_lower_emutls);
@@ -161,6 +161,7 @@  along with GCC; see the file COPYING3.
   NEXT_PASS (pass_ipa_fn_summary);
   NEXT_PASS (pass_ipa_inline);
   NEXT_PASS (pass_ipa_pure_const);
+  NEXT_PASS (pass_ipa_free_fn_summary, false /* small_p */);
   NEXT_PASS (pass_ipa_reference);
   /* This pass needs to be scheduled after any IP code duplication.   */
   NEXT_PASS (pass_ipa_single_use);
--- gcc/ipa-pure-const.c.jj	2017-12-04 22:29:11.000000000 +0100
+++ gcc/ipa-pure-const.c	2017-12-20 11:33:11.905956408 +0100
@@ -2013,10 +2013,6 @@  execute (function *)
     if (has_function_state (node))
       free (get_function_state (node));
   funct_state_vec.release ();
-
-  /* In WPA we use inline summaries for partitioning process.  */
-  if (!flag_wpa)
-    ipa_free_fn_summary ();
   return remove_p ? TODO_remove_functions : 0;
 }
 
--- gcc/testsuite/gcc.dg/pr83506.c.jj	2017-12-20 11:39:48.405212526 +0100
+++ gcc/testsuite/gcc.dg/pr83506.c	2017-12-20 11:39:18.000000000 +0100
@@ -0,0 +1,14 @@ 
+/* PR ipa/83506 */
+/* { dg-do compile { target pthread } } */
+/* { dg-options "-O1 -ftree-parallelize-loops=2 -fno-ipa-pure-const" } */
+
+unsigned int
+foo (unsigned int x, int y)
+{
+  while (y < 1)
+    {
+      x *= 3;
+      ++y;
+    }
+  return x;
+}
--- gcc/testsuite/gcc.dg/ipa/ctor-empty-1.c.jj	2017-05-24 11:59:01.000000000 +0200
+++ gcc/testsuite/gcc.dg/ipa/ctor-empty-1.c	2017-12-20 20:15:27.144761517 +0100
@@ -1,7 +1,7 @@ 
 /* { dg-do compile } */
-/* { dg-options "-O3 -c -fdump-ipa-free-fnsummary"  } */
+/* { dg-options "-O3 -c -fdump-ipa-free-fnsummary1"  } */
 static __attribute__((constructor))
 void empty_constructor()
 {
 }
-/* { dg-final { scan-ipa-dump "Reclaiming functions: empty_constructor"  "free-fnsummary"  } } */
+/* { dg-final { scan-ipa-dump "Reclaiming functions: empty_constructor"  "free-fnsummary1"  } } */