Re: [PATCH, PR81030] Call compute_outgoing_frequencies at expand

[Jan Hubicka <hubicka@ucw.cz>]

Hi,
thanks for looking into this issue.  It is quite weird indeed.
> Commenting out this bit makes the compilation succeed.

so my understanding is that RTL expansions confuses itself and redistributes
probabilities to two jumps while immediately optimizing out the second...

I think in such case instead of calling compute_outgoing_frequencies which
will take confused probability from REG_BR_PROB_NOTE and produce inconsistent
profile, it is better to take the existing probabilities in profile and put
them back to RTL.

Of course it would be nice to teach RTl expansion to not do such mistakes
(because the patch bellow helps only in case the BB happens to not split at
all), but that would be quite difficult I guess.

I am testing the attached patch.

Honza

Index: cfgbuild.c
===================================================================
--- cfgbuild.c	(revision 250246)
+++ cfgbuild.c	(working copy)
@@ -676,7 +676,14 @@ find_many_sub_basic_blocks (sbitmap bloc
 	else
 	  /* If nothing changed, there is no need to create new BBs.  */
 	  if (EDGE_COUNT (bb->succs) == n_succs[bb->index])
-	    continue;
+	    {
+	      /* In rare occassions RTL expansion might have mistakely assigned
+		 a probabilities different from what is in CFG.  This happens
+		 when we try to split branch to two but optimize out the
+		 second branch during the way. See PR81030.  */
+	      update_br_prob_note (bb);
+	      continue;
+	    }
 
 	compute_outgoing_frequencies (bb);
       }
> 
> 
> III.
> 
> The bit added in find_many_sub_basic_blocks makes sense to me.
> 
> It just seems to me that we have been relying on find_many_sub_basic_blocks
> to call compute_outgoing_frequencies for all basic blocks during expand.
> This patch restores that situation, and fixes the ICE.
> 
> Bootstrapped and reg-tested on x86_64.
> 
> OK for trunk?
> 
> Thanks,
> - Tom

> Call compute_outgoing_frequencies at expand
> 
> 2017-07-17  Tom de Vries  <tom@codesourcery.com>
> 
> 	PR middle-end/81030
> 	* cfgbuild.c (find_many_sub_basic_blocks): Add and handle update_freq
> 	parameter.
> 	* cfgbuild.h (find_many_sub_basic_blocks): Add update_freq parameter.
> 	* cfgexpand.c (pass_expand::execute): Call find_many_sub_basic_blocks
> 	with update_freq == true.
> 
> 	* gcc.dg/pr81030.c: New test.
> 
> ---
>  gcc/cfgbuild.c                 |  4 ++--
>  gcc/cfgbuild.h                 |  3 ++-
>  gcc/cfgexpand.c                |  2 +-
>  gcc/testsuite/gcc.dg/pr81030.c | 29 +++++++++++++++++++++++++++++
>  4 files changed, 34 insertions(+), 4 deletions(-)
> 
> diff --git a/gcc/cfgbuild.c b/gcc/cfgbuild.c
> index 56a2cb9..78036fe 100644
> --- a/gcc/cfgbuild.c
> +++ b/gcc/cfgbuild.c
> @@ -594,7 +594,7 @@ compute_outgoing_frequencies (basic_block b)
>     and create edges.  */
>  
>  void
> -find_many_sub_basic_blocks (sbitmap blocks)
> +find_many_sub_basic_blocks (sbitmap blocks, bool update_freq)
>  {
>    basic_block bb, min, max;
>    bool found = false;
> @@ -677,7 +677,7 @@ find_many_sub_basic_blocks (sbitmap blocks)
>  	  }
>  	else
>  	  /* If nothing changed, there is no need to create new BBs.  */
> -	  if (EDGE_COUNT (bb->succs) == n_succs[bb->index])
> +	  if (!update_freq && EDGE_COUNT (bb->succs) == n_succs[bb->index])
>  	    continue;
>  
>  	compute_outgoing_frequencies (bb);
> diff --git a/gcc/cfgbuild.h b/gcc/cfgbuild.h
> index afda5ac..0c58590 100644
> --- a/gcc/cfgbuild.h
> +++ b/gcc/cfgbuild.h
> @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3.  If not see
>  extern bool inside_basic_block_p (const rtx_insn *);
>  extern bool control_flow_insn_p (const rtx_insn *);
>  extern void rtl_make_eh_edge (sbitmap, basic_block, rtx);
> -extern void find_many_sub_basic_blocks (sbitmap);
> +extern void find_many_sub_basic_blocks (sbitmap block,
> +					bool update_freq = false);
>  
>  #endif /* GCC_CFGBUILD_H */
> diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> index 3e1d24d..e030a86 100644
> --- a/gcc/cfgexpand.c
> +++ b/gcc/cfgexpand.c
> @@ -6464,7 +6464,7 @@ pass_expand::execute (function *fun)
>  
>    auto_sbitmap blocks (last_basic_block_for_fn (fun));
>    bitmap_ones (blocks);
> -  find_many_sub_basic_blocks (blocks);
> +  find_many_sub_basic_blocks (blocks, true);
>    purge_all_dead_edges ();
>  
>    expand_stack_alignment ();
> diff --git a/gcc/testsuite/gcc.dg/pr81030.c b/gcc/testsuite/gcc.dg/pr81030.c
> new file mode 100644
> index 0000000..23da6e5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr81030.c
> @@ -0,0 +1,29 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O1" } */
> +
> +void __assert_fail (const char *, const char *, unsigned int, const char *);
> +
> +int a, b, c, d, e, f, h;
> +unsigned char g;
> +
> +int main ()
> +{
> +  int i, *j = &b;
> +  if (a)
> +    {
> +      if (h)
> +	{
> +	  int **k = &j;
> +	  d = 0;
> +	  *k = &e;
> +	}
> +      else
> +	for (b = 0; b > -28; b = g)
> +	  ;
> +      c || !j ? : __assert_fail ("c || !j", "small.c", 20, "main");
> +      if (f)
> +	for (i = 0; i < 1; i++)
> +	  ;
> +    }
> +  return 0;
> +}