Re: [PATCH] Fix ICE with -ftree-parallelize-loops=2 -fno-ipa-pure-const (PR ipa/83506)

[David Malcolm <dmalcolm at redhat dot com>]

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