Re: [PATCH 2/2] Split edge when edge locus and dest don't match

["Martin Liška" <mliska@suse.cz>]

On 10/5/22 14:04, Jørgen Kvalsvik via Gcc-patches wrote:
> Edges with locus are candidates for splitting so that the edge with
> locus is the only edge out of a basic block, except when the locuses
> match. The test checks the last (non-debug) statement in a basic block,
> but this causes an unnecessary split when the edge locus go to a block
> which coincidentally has an unrelated label. Comparing the first
> statement of the destination block is the same check but does not get
> tripped up by labels.
> 
> An example with source/edge/dest locus when an edge is split:
> 
>       1  int fn (int a, int b, int c) {
>       2        int x = 0;
>       3        if (a && b) {
>       4                x = a;
>       5        } else {
>       6  a_:
>       7            x = (a - b);
>       8        }
>       9
>      10        return x;
>      11  }
> 
> block  file  line   col  stmt
> 
> src     t.c     3    10  if (a_3(D) != 0)
> edge    t.c     6     1
> dest    t.c     6     1  a_:
> 
> src     t.c     3    13  if (b_4(D) != 0)
> edge    t.c     6     1
> dst     t.c     6     1  a_:
> 
> With label removed:
> 
>       1  int fn (int a, int b, int c) {
>       2        int x = 0;
>       3        if (a && b) {
>       4                x = a;
>       5        } else {
>       6  // a_: <- label removed
>       7            x = (a - b);
>       8        }
>       9
>      10        return x;
>      11
> 
> block  file  line   col  stmt
> 
> src     t.c     3    10  if (a_3(D) != 0)
> edge  (null)    0     0
> dest    t.c     6     1  a_:
> 
> src     t.c     3    13  if (b_4(D) != 0)
> edge  (null)    0     0
> dst     t.c     6     1  a_:
> 
> and this is extract from gcov-4b.c which *should* split:
> 
>     205  int
>     206  test_switch (int i, int j)
>     207  {
>     208    int result = 0;
>     209
>     210    switch (i)                            /* branch(80 25) */
>     211                                          /* branch(end) */
>     212      {
>     213        case 1:
>     214          result = do_something (2);
>     215          break;
>     216        case 2:
>     217          result = do_something (1024);
>     218          break;
>     219        case 3:
>     220        case 4:
>     221          if (j == 2)                     /* branch(67) */
>     222                                          /* branch(end) */
>     223            return do_something (4);
>     224          result = do_something (8);
>     225          break;
>     226        default:
>     227          result = do_something (32);
>     228          switch_m++;
>     229          break;
>     230      }
>     231    return result;
>     231  }
> 
> block  file  line   col  stmt
> 
> src    4b.c   214    18  result_18 = do_something (2);
> edge   4b.c   215     9
> dst    4b.c   231    10  _22 = result_3;
> 
> src    4b.c   217    18  result_16 = do_something (1024);
> edge   4b.c   218     9
> dst    4b.c   231    10  _22 = result_3;
> 
> src    4b.c   224    18  result_12 = do_something (8);
> edge   4b.c   225     9
> dst    4b.c   231    10  _22 = result_3;
> 
> Note that the behaviour of comparison is preserved for the (switch) edge
> splitting case. The former case now fails the check (even though
> e->goto_locus is no longer a reserved location) because the dest matches
> the e->locus.

It's fine, please install it.
I verified tramp3d coverage output is the same as before the patch.

Martin 

> 
> gcc/ChangeLog:
> 
>         * profile.cc (branch_prob): Compare edge locus to dest, not src.
> ---
>  gcc/profile.cc | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/gcc/profile.cc b/gcc/profile.cc
> index 96121d60711..c13a79a84c2 100644
> --- a/gcc/profile.cc
> +++ b/gcc/profile.cc
> @@ -1208,17 +1208,17 @@ branch_prob (bool thunk)
>  	  FOR_EACH_EDGE (e, ei, bb->succs)
>  	    {
>  	      gimple_stmt_iterator gsi;
> -	      gimple *last = NULL;
> +	      gimple *dest = NULL;
>  
>  	      /* It may happen that there are compiler generated statements
>  		 without a locus at all.  Go through the basic block from the
>  		 last to the first statement looking for a locus.  */
> -	      for (gsi = gsi_last_nondebug_bb (bb);
> +	      for (gsi = gsi_start_nondebug_bb (bb);
>  		   !gsi_end_p (gsi);
> -		   gsi_prev_nondebug (&gsi))
> +		   gsi_next_nondebug (&gsi))
>  		{
> -		  last = gsi_stmt (gsi);
> -		  if (!RESERVED_LOCATION_P (gimple_location (last)))
> +		  dest = gsi_stmt (gsi);
> +		  if (!RESERVED_LOCATION_P (gimple_location (dest)))
>  		    break;
>  		}
>  
> @@ -1227,14 +1227,14 @@ branch_prob (bool thunk)
>  		 Don't do that when the locuses match, so
>  		 if (blah) goto something;
>  		 is not computed twice.  */
> -	      if (last
> -		  && gimple_has_location (last)
> +	      if (dest
> +		  && gimple_has_location (dest)
>  		  && !RESERVED_LOCATION_P (e->goto_locus)
>  		  && !single_succ_p (bb)
>  		  && (LOCATION_FILE (e->goto_locus)
> -		      != LOCATION_FILE (gimple_location (last))
> +		      != LOCATION_FILE (gimple_location (dest))
>  		      || (LOCATION_LINE (e->goto_locus)
> -			  != LOCATION_LINE (gimple_location (last)))))
> +			  != LOCATION_LINE (gimple_location (dest)))))
>  		{
>  		  basic_block new_bb = split_edge (e);
>  		  edge ne = single_succ_edge (new_bb);