[PATCH 0/2] gcov: Split when edge locus differ from dest bb

[PATCH 0/2] gcov: Split when edge locus differ from dest bb

[Jørgen Kvalsvik]

Original discussion: https://gcc.gnu.org/pipermail/gcc/2022-October/239544.html

Some tiny test additions as well as more accurate check for when to
split edges for coverage. This patch preserves the edge splitting
heuristic except using a slightly more precise comparison, but makes a
huge difference for the condition profiling [1].

I compared the .gcov files for the gcc and g++ testsuites with and
without this patch and found no differences, and bootstrapped on
x86_64-linux.

[1] https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598165.html

Jørgen Kvalsvik (2):
  gcov: test switch/break line counts
  Split edge when edge locus and dest don't match

 gcc/profile.cc                        | 18 +++++++++---------
 gcc/testsuite/g++.dg/gcov/gcov-1.C    |  8 ++++----
 gcc/testsuite/gcc.misc-tests/gcov-4.c |  4 ++--
 3 files changed, 15 insertions(+), 15 deletions(-)

-- 
2.30.2

[PATCH 1/2] gcov: test switch/break line counts

[Jørgen Kvalsvik]

The coverage support will under some conditions decide to split edges to
accurately report coverage. By running the test suite with/without this
edge splitting a small diff shows up, addressed by this patch, which
should catch future regressions.

Removing the edge splitting:

diff --git a/gcc/profile.cc b/gcc/profile.cc
--- a/gcc/profile.cc
+++ b/gcc/profile.cc
@@ -1244,19 +1244,7 @@ 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)
-                 && !RESERVED_LOCATION_P (e->goto_locus)
-                 && !single_succ_p (bb)
-                 && (LOCATION_FILE (e->goto_locus)
-                     != LOCATION_FILE (gimple_location (last))
-                     || (LOCATION_LINE (e->goto_locus)
-                         != LOCATION_LINE (gimple_location (last)))))
-               {
-                 basic_block new_bb = split_edge (e);
-                 edge ne = single_succ_edge (new_bb);
-                 ne->goto_locus = e->goto_locus;
-               }
+
        if ((e->flags & (EDGE_ABNORMAL | EDGE_ABNORMAL_CALL))
                && e->dest != EXIT_BLOCK_PTR_FOR_FN (cfun))
                need_exit_edge = 1;

Assuming the .gcov files from make chec-gcc RUNTESTFLAGS=gcov.exp are
kept:

$ diff -r no-split-edge with-split-edge | grep -C 2 -E "^[<>]\s\s"
diff -r sans-split-edge/gcc/gcov-4.c.gcov with-split-edge/gcc/gcov-4.c.gcov
   228c228
   <         -:  224:        break;
   ---
   >         1:  224:        break;
   231c231
   <         -:  227:        break;
   ---
   >     #####:  227:        break;
   237c237
   <         -:  233:        break;
   ---
   >         2:  233:        break;

gcc/testsuite/ChangeLog:

	* g++.dg/gcov/gcov-1.C: Add line count check.
	* gcc.misc-tests/gcov-4.c: Likewise.
---
 gcc/testsuite/g++.dg/gcov/gcov-1.C    | 8 ++++----
 gcc/testsuite/gcc.misc-tests/gcov-4.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
index 9018b9a3a73..ee383b480a8 100644
--- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
+++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
@@ -257,20 +257,20 @@ test_switch (int i, int j)
   switch (i)				/* count(5) */
 					/* branch(end) */
     {
-      case 1:
+      case 1:				/* count(1) */
         result = do_something (2);	/* count(1) */
-        break;
+        break;				/* count(1) */
       case 2:
         result = do_something (1024);
         break;
-      case 3:
+      case 3:				/* count(3) */
       case 4:
 					/* branch(67) */
         if (j == 2)			/* count(3) */
 					/* branch(end) */
           return do_something (4);	/* count(1) */
         result = do_something (8);	/* count(2) */
-        break;
+        break;				/* count(2) */
       default:
 	result = do_something (32);	/* count(1) */
 	switch_m++;			/* count(1) */
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c
index 9d8ab1c1097..498d299b66b 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
@@ -221,7 +221,7 @@ test_switch (int i, int j)
     {
       case 1:
         result = do_something (2);	/* count(1) */
-        break;
+        break;				/* count(1) */
       case 2:
         result = do_something (1024);
         break;
@@ -230,7 +230,7 @@ test_switch (int i, int j)
         if (j == 2)			/* count(3) */
           return do_something (4);	/* count(1) */
         result = do_something (8);	/* count(2) */
-        break;
+        break;				/* count(2) */
       default:
 	result = do_something (32);	/* count(1) */
 	switch_m++;			/* count(1) */
-- 
2.30.2

Re: [PATCH 1/2] gcov: test switch/break line counts

[Martin Liška]

On 10/5/22 14:04, Jørgen Kvalsvik via Gcc-patches wrote:
> The coverage support will under some conditions decide to split edges to
> accurately report coverage. By running the test suite with/without this
> edge splitting a small diff shows up, addressed by this patch, which
> should catch future regressions.

Thanks for the patch, it's OK (please apply it).

Martin

> 
> Removing the edge splitting:
> 
> diff --git a/gcc/profile.cc b/gcc/profile.cc
> --- a/gcc/profile.cc
> +++ b/gcc/profile.cc
> @@ -1244,19 +1244,7 @@ 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)
> -                 && !RESERVED_LOCATION_P (e->goto_locus)
> -                 && !single_succ_p (bb)
> -                 && (LOCATION_FILE (e->goto_locus)
> -                     != LOCATION_FILE (gimple_location (last))
> -                     || (LOCATION_LINE (e->goto_locus)
> -                         != LOCATION_LINE (gimple_location (last)))))
> -               {
> -                 basic_block new_bb = split_edge (e);
> -                 edge ne = single_succ_edge (new_bb);
> -                 ne->goto_locus = e->goto_locus;
> -               }
> +
>         if ((e->flags & (EDGE_ABNORMAL | EDGE_ABNORMAL_CALL))
>                 && e->dest != EXIT_BLOCK_PTR_FOR_FN (cfun))
>                 need_exit_edge = 1;
> 
> Assuming the .gcov files from make chec-gcc RUNTESTFLAGS=gcov.exp are
> kept:
> 
> $ diff -r no-split-edge with-split-edge | grep -C 2 -E "^[<>]\s\s"
> diff -r sans-split-edge/gcc/gcov-4.c.gcov with-split-edge/gcc/gcov-4.c.gcov
>    228c228
>    <         -:  224:        break;
>    ---
>    >         1:  224:        break;
>    231c231
>    <         -:  227:        break;
>    ---
>    >     #####:  227:        break;
>    237c237
>    <         -:  233:        break;
>    ---
>    >         2:  233:        break;
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/gcov/gcov-1.C: Add line count check.
> 	* gcc.misc-tests/gcov-4.c: Likewise.
> ---
>  gcc/testsuite/g++.dg/gcov/gcov-1.C    | 8 ++++----
>  gcc/testsuite/gcc.misc-tests/gcov-4.c | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
> index 9018b9a3a73..ee383b480a8 100644
> --- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
> +++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
> @@ -257,20 +257,20 @@ test_switch (int i, int j)
>    switch (i)				/* count(5) */
>  					/* branch(end) */
>      {
> -      case 1:
> +      case 1:				/* count(1) */
>          result = do_something (2);	/* count(1) */
> -        break;
> +        break;				/* count(1) */
>        case 2:
>          result = do_something (1024);
>          break;
> -      case 3:
> +      case 3:				/* count(3) */
>        case 4:
>  					/* branch(67) */
>          if (j == 2)			/* count(3) */
>  					/* branch(end) */
>            return do_something (4);	/* count(1) */
>          result = do_something (8);	/* count(2) */
> -        break;
> +        break;				/* count(2) */
>        default:
>  	result = do_something (32);	/* count(1) */
>  	switch_m++;			/* count(1) */
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c
> index 9d8ab1c1097..498d299b66b 100644
> --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
> @@ -221,7 +221,7 @@ test_switch (int i, int j)
>      {
>        case 1:
>          result = do_something (2);	/* count(1) */
> -        break;
> +        break;				/* count(1) */
>        case 2:
>          result = do_something (1024);
>          break;
> @@ -230,7 +230,7 @@ test_switch (int i, int j)
>          if (j == 2)			/* count(3) */
>            return do_something (4);	/* count(1) */
>          result = do_something (8);	/* count(2) */
> -        break;
> +        break;				/* count(2) */
>        default:
>  	result = do_something (32);	/* count(1) */
>  	switch_m++;			/* count(1) */

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

[Jørgen Kvalsvik]

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.

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);
-- 
2.30.2

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

[Martin Liška]

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);

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

[Richard Biener]

On Wed, Oct 5, 2022 at 2:49 PM Martin Liška <mliska@suse.cz> wrote:
>
> 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.  */

The comment no longer matches the code.

> > -           for (gsi = gsi_last_nondebug_bb (bb);
> > +           for (gsi = gsi_start_nondebug_bb (bb);

^^^ and you are looking at the branch block stmts (bb), not the destination
block stmts (e->dest)

> >                  !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)))))

this heuristic needs to be in sync with how we insert edge counters
which seems to be hidden in the MST compute (and/or edge insertion).
I don't see how it's a win to disregard 'last' and only consider 'dest' here.

I think the patch is wrong.  Please revert if you applied it already.

Thanks,
Richard.

> >               {
> >                 basic_block new_bb = split_edge (e);
> >                 edge ne = single_succ_edge (new_bb);
>

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

[Jørgen Kvalsvik]

On 06/10/2022 10:12, Richard Biener wrote:
> On Wed, Oct 5, 2022 at 2:49 PM Martin Liška <mliska@suse.cz> wrote:
>>
>> 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.  */
> 
> The comment no longer matches the code.>
>>> -           for (gsi = gsi_last_nondebug_bb (bb);
>>> +           for (gsi = gsi_start_nondebug_bb (bb);
> 
> ^^^ and you are looking at the branch block stmts (bb), not the destination
> block stmts (e->dest)
> 
>>>                  !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)))))
> 
> this heuristic needs to be in sync with how we insert edge counters
> which seems to be hidden in the MST compute (and/or edge insertion).
> I don't see how it's a win to disregard 'last' and only consider 'dest' here.
> 
> I think the patch is wrong.  Please revert if you applied it already.

I haven't applied it yet, so unless someone beat me to it there's fortunately
nothing to revert.

> I don't see how it's a win to disregard 'last' and only consider 'dest' here.

It might not be other than that it unbreaks my condition profiling by not
splitting condition edges and apparently doesn't cause a regression (one caught
by tests anyway). That being said the heuristic may very well be wrong (as is
the implementation since it considers bb and not e->dest, although I'm sure I
tested it with e->dest at some point).

I guess the most important question is if the if (a && b) {} {label:} edges
should be split in the first place. As mentioned in the patch set, the only
difference in the test suite happens on break in switches. I'll tinker a bit
more to see if I can figure out what's going on or if the heuristic can
otherwise be improved.

Then, when does a block with a goto_locus edge have multiple successors? From my
previous testing it doesn't seem like it's the conditions make a goto_locus, but
it's more than just plain gotos right? When would it then have multiple
successors? Switches and exception handling? If that's the case then maybe the
heuristic can be improved by simply checking the edge type.

Thanks,
Jørgen


> 
> Thanks,
> Richard.
> 
>>>               {
>>>                 basic_block new_bb = split_edge (e);
>>>                 edge ne = single_succ_edge (new_bb);
>>

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

[Richard Biener]

On Thu, Oct 6, 2022 at 4:28 PM Jørgen Kvalsvik
<jorgen.kvalsvik@woven-planet.global> wrote:
>
> On 06/10/2022 10:12, Richard Biener wrote:
> > On Wed, Oct 5, 2022 at 2:49 PM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> 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.  */
> >
> > The comment no longer matches the code.>
> >>> -           for (gsi = gsi_last_nondebug_bb (bb);
> >>> +           for (gsi = gsi_start_nondebug_bb (bb);
> >
> > ^^^ and you are looking at the branch block stmts (bb), not the destination
> > block stmts (e->dest)
> >
> >>>                  !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)))))
> >
> > this heuristic needs to be in sync with how we insert edge counters
> > which seems to be hidden in the MST compute (and/or edge insertion).
> > I don't see how it's a win to disregard 'last' and only consider 'dest' here.
> >
> > I think the patch is wrong.  Please revert if you applied it already.
>
> I haven't applied it yet, so unless someone beat me to it there's fortunately
> nothing to revert.
>
> > I don't see how it's a win to disregard 'last' and only consider 'dest' here.
>
> It might not be other than that it unbreaks my condition profiling by not
> splitting condition edges and apparently doesn't cause a regression (one caught
> by tests anyway). That being said the heuristic may very well be wrong (as is
> the implementation since it considers bb and not e->dest, although I'm sure I
> tested it with e->dest at some point).
>
> I guess the most important question is if the if (a && b) {} {label:} edges
> should be split in the first place. As mentioned in the patch set, the only
> difference in the test suite happens on break in switches. I'll tinker a bit
> more to see if I can figure out what's going on or if the heuristic can
> otherwise be improved.
>
> Then, when does a block with a goto_locus edge have multiple successors? From my
> previous testing it doesn't seem like it's the conditions make a goto_locus, but
> it's more than just plain gotos right? When would it then have multiple
> successors? Switches and exception handling? If that's the case then maybe the
> heuristic can be improved by simply checking the edge type.

The goto_locus of an edge is usually either the locus of the control stmt or the
locus of the stmt the control transfers to.  The most important case is for
'goto' stmts themselves since those are elided and become edges (thus
'goto_locus').

My understanding as of why we split edges at all is that we want to instrument
different locations with different counters and since we cannot have counters on
edges itself but have to either insert the counter on the source or
the destination
we in some cases have to split the edge to create an insert location
to not falsely
account.  instrument_edges seems to simply use gsi_insert_on_edge which
inserts with the gimple_find_edge_insert_loc logic which doesn't look
at goto_locus
at all.  So the existing heuristic must be fragile as well.

BUT - as you say, the plain goto shouldn't be subject to edge instrumentation.
The interesting case is probably computed goto (which has multiple successors)
and from what I can see a branch where we take the goto_locus from the
COND_EXPRs then/else goto stmt which in theory could have different locations.

I don't fully understand your requirement of not splitting edges -
I'll just note that
the place you are patching is not the only place where edges are split (but
the insert location computation only sees those splits).

Richard.

> Thanks,
> Jørgen
>
>
> >
> > Thanks,
> > Richard.
> >
> >>>               {
> >>>                 basic_block new_bb = split_edge (e);
> >>>                 edge ne = single_succ_edge (new_bb);
> >>

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

[Jørgen Kvalsvik]

On 07/10/2022 08:53, Richard Biener wrote:
> On Thu, Oct 6, 2022 at 4:28 PM Jørgen Kvalsvik
> <jorgen.kvalsvik@woven-planet.global> wrote:
>>
>> On 06/10/2022 10:12, Richard Biener wrote:
>>> On Wed, Oct 5, 2022 at 2:49 PM Martin Liška <mliska@suse.cz> wrote:
>>>>
>>>> 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.  */
>>>
>>> The comment no longer matches the code.>
>>>>> -           for (gsi = gsi_last_nondebug_bb (bb);
>>>>> +           for (gsi = gsi_start_nondebug_bb (bb);
>>>
>>> ^^^ and you are looking at the branch block stmts (bb), not the destination
>>> block stmts (e->dest)
>>>
>>>>>                  !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)))))
>>>
>>> this heuristic needs to be in sync with how we insert edge counters
>>> which seems to be hidden in the MST compute (and/or edge insertion).
>>> I don't see how it's a win to disregard 'last' and only consider 'dest' here.
>>>
>>> I think the patch is wrong.  Please revert if you applied it already.
>>
>> I haven't applied it yet, so unless someone beat me to it there's fortunately
>> nothing to revert.
>>
>>> I don't see how it's a win to disregard 'last' and only consider 'dest' here.
>>
>> It might not be other than that it unbreaks my condition profiling by not
>> splitting condition edges and apparently doesn't cause a regression (one caught
>> by tests anyway). That being said the heuristic may very well be wrong (as is
>> the implementation since it considers bb and not e->dest, although I'm sure I
>> tested it with e->dest at some point).
>>
>> I guess the most important question is if the if (a && b) {} {label:} edges
>> should be split in the first place. As mentioned in the patch set, the only
>> difference in the test suite happens on break in switches. I'll tinker a bit
>> more to see if I can figure out what's going on or if the heuristic can
>> otherwise be improved.
>>
>> Then, when does a block with a goto_locus edge have multiple successors? From my
>> previous testing it doesn't seem like it's the conditions make a goto_locus, but
>> it's more than just plain gotos right? When would it then have multiple
>> successors? Switches and exception handling? If that's the case then maybe the
>> heuristic can be improved by simply checking the edge type.
> 
> The goto_locus of an edge is usually either the locus of the control stmt or the
> locus of the stmt the control transfers to.  The most important case is for
> 'goto' stmts themselves since those are elided and become edges (thus
> 'goto_locus').
> 
> My understanding as of why we split edges at all is that we want to instrument
> different locations with different counters and since we cannot have counters on
> edges itself but have to either insert the counter on the source or
> the destination
> we in some cases have to split the edge to create an insert location
> to not falsely
> account.  instrument_edges seems to simply use gsi_insert_on_edge which
> inserts with the gimple_find_edge_insert_loc logic which doesn't look
> at goto_locus
> at all.  So the existing heuristic must be fragile as well.
> 
> BUT - as you say, the plain goto shouldn't be subject to edge instrumentation.
> The interesting case is probably computed goto (which has multiple successors)
> and from what I can see a branch where we take the goto_locus from the
> COND_EXPRs then/else goto stmt which in theory could have different locations.
> 
> I don't fully understand your requirement of not splitting edges -
> I'll just note that
> the place you are patching is not the only place where edges are split (but
> the insert location computation only sees those splits).
> 
> Richard.

Ok, I think I understand. To move forward I propose this additional test case,
if anything to catch regressions. Naturally, it fails when the split does not
happen because the 'first' label gets incremented twice (I'll probably rename it
pre application, assuming it gets approved) not once.

This also means I need to change strategy for condition coverage - either, I
must snapshot these splits and make a mapping table for the "original"
identities or maybe run the analysis before this splitting happens.


diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c
b/gcc/testsuite/gcc.misc-tests/gcov-4.c
index 498d299b66b..b1dc29b573a 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
@@ -110,6 +110,29 @@ lab2:
   return 8;				/* count(1) */
 }

+int
+test_goto3 (int i, int j)
+{
+    if (j) goto first;		/* count(1) */
+
+top:
+    if (i)			/* count(1) */
+      {
+	i = do_something (i);
+      }
+    else
+      {
+first:				/* count(1) */
+	j = do_something (j);	/* count(2) */
+	if (j)			/* count(2) */
+	  {
+	    j = 0;		/* count(1) */
+	    goto top;		/* count(1) */
+	  }
+      }
+    return 16;
+}
+
 void
 call_goto ()
 {
@@ -117,6 +140,7 @@ call_goto ()
   goto_val += test_goto1 (1);
   goto_val += test_goto2 (3);
   goto_val += test_goto2 (30);
+  goto_val += test_goto3 (0, 1);
 }

 /* Check nested if-then-else statements. */
@@ -260,7 +284,7 @@ main()
   call_unref ();
   if ((for_val1 != 12)
       || (for_val2 != 87)
-      || (goto_val != 15)
+      || (goto_val != 31)
       || (ifelse_val1 != 31)
       || (ifelse_val2 != 23)
       || (ifelse_val3 != 246)

What do you think?

Thanks,
Jørgen

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

[Jørgen Kvalsvik]

On 07/10/2022 13:45, Jørgen Kvalsvik wrote:
> On 07/10/2022 08:53, Richard Biener wrote:
>> On Thu, Oct 6, 2022 at 4:28 PM Jørgen Kvalsvik
>> <jorgen.kvalsvik@woven-planet.global> wrote:
>>>
>>> On 06/10/2022 10:12, Richard Biener wrote:
>>>> On Wed, Oct 5, 2022 at 2:49 PM Martin Liška <mliska@suse.cz> wrote:
>>>>>
>>>>> 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.  */
>>>>
>>>> The comment no longer matches the code.>
>>>>>> -           for (gsi = gsi_last_nondebug_bb (bb);
>>>>>> +           for (gsi = gsi_start_nondebug_bb (bb);
>>>>
>>>> ^^^ and you are looking at the branch block stmts (bb), not the destination
>>>> block stmts (e->dest)
>>>>
>>>>>>                  !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)))))
>>>>
>>>> this heuristic needs to be in sync with how we insert edge counters
>>>> which seems to be hidden in the MST compute (and/or edge insertion).
>>>> I don't see how it's a win to disregard 'last' and only consider 'dest' here.
>>>>
>>>> I think the patch is wrong.  Please revert if you applied it already.
>>>
>>> I haven't applied it yet, so unless someone beat me to it there's fortunately
>>> nothing to revert.
>>>
>>>> I don't see how it's a win to disregard 'last' and only consider 'dest' here.
>>>
>>> It might not be other than that it unbreaks my condition profiling by not
>>> splitting condition edges and apparently doesn't cause a regression (one caught
>>> by tests anyway). That being said the heuristic may very well be wrong (as is
>>> the implementation since it considers bb and not e->dest, although I'm sure I
>>> tested it with e->dest at some point).
>>>
>>> I guess the most important question is if the if (a && b) {} {label:} edges
>>> should be split in the first place. As mentioned in the patch set, the only
>>> difference in the test suite happens on break in switches. I'll tinker a bit
>>> more to see if I can figure out what's going on or if the heuristic can
>>> otherwise be improved.
>>>
>>> Then, when does a block with a goto_locus edge have multiple successors? From my
>>> previous testing it doesn't seem like it's the conditions make a goto_locus, but
>>> it's more than just plain gotos right? When would it then have multiple
>>> successors? Switches and exception handling? If that's the case then maybe the
>>> heuristic can be improved by simply checking the edge type.
>>
>> The goto_locus of an edge is usually either the locus of the control stmt or the
>> locus of the stmt the control transfers to.  The most important case is for
>> 'goto' stmts themselves since those are elided and become edges (thus
>> 'goto_locus').
>>
>> My understanding as of why we split edges at all is that we want to instrument
>> different locations with different counters and since we cannot have counters on
>> edges itself but have to either insert the counter on the source or
>> the destination
>> we in some cases have to split the edge to create an insert location
>> to not falsely
>> account.  instrument_edges seems to simply use gsi_insert_on_edge which
>> inserts with the gimple_find_edge_insert_loc logic which doesn't look
>> at goto_locus
>> at all.  So the existing heuristic must be fragile as well.
>>
>> BUT - as you say, the plain goto shouldn't be subject to edge instrumentation.
>> The interesting case is probably computed goto (which has multiple successors)
>> and from what I can see a branch where we take the goto_locus from the
>> COND_EXPRs then/else goto stmt which in theory could have different locations.
>>
>> I don't fully understand your requirement of not splitting edges -
>> I'll just note that
>> the place you are patching is not the only place where edges are split (but
>> the insert location computation only sees those splits).
>>
>> Richard.
> 
> Ok, I think I understand. To move forward I propose this additional test case,
> if anything to catch regressions. Naturally, it fails when the split does not
> happen because the 'first' label gets incremented twice (I'll probably rename it
> pre application, assuming it gets approved) not once.
> 
> This also means I need to change strategy for condition coverage - either, I
> must snapshot these splits and make a mapping table for the "original"
> identities or maybe run the analysis before this splitting happens.
> 
> 
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c
> b/gcc/testsuite/gcc.misc-tests/gcov-4.c
> index 498d299b66b..b1dc29b573a 100644
> --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
> @@ -110,6 +110,29 @@ lab2:
>    return 8;				/* count(1) */
>  }
> 
> +int
> +test_goto3 (int i, int j)
> +{
> +    if (j) goto first;		/* count(1) */
> +
> +top:
> +    if (i)			/* count(1) */
> +      {
> +	i = do_something (i);
> +      }
> +    else
> +      {
> +first:				/* count(1) */
> +	j = do_something (j);	/* count(2) */
> +	if (j)			/* count(2) */
> +	  {
> +	    j = 0;		/* count(1) */
> +	    goto top;		/* count(1) */
> +	  }
> +      }
> +    return 16;
> +}
> +
>  void
>  call_goto ()
>  {
> @@ -117,6 +140,7 @@ call_goto ()
>    goto_val += test_goto1 (1);
>    goto_val += test_goto2 (3);
>    goto_val += test_goto2 (30);
> +  goto_val += test_goto3 (0, 1);
>  }
> 
>  /* Check nested if-then-else statements. */
> @@ -260,7 +284,7 @@ main()
>    call_unref ();
>    if ((for_val1 != 12)
>        || (for_val2 != 87)
> -      || (goto_val != 15)
> +      || (goto_val != 31)
>        || (ifelse_val1 != 31)
>        || (ifelse_val2 != 23)
>        || (ifelse_val3 != 246)
> 
> What do you think?
> 
> Thanks,
> Jørgen

Hello,

After tinkering a bit more I figured out a patch I could do to merge these
splits in the condition coverage code, rather than relying on the splits not
happening. I still think the tests are a good addition, but there's no longer a
reason for me to change the splitting heuristics.

Should I prepare a separate patch set for the tests?

Thanks,
Jørgen

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

[Richard Biener]

On Tue, Oct 11, 2022 at 12:57 PM Jørgen Kvalsvik
<jorgen.kvalsvik@woven-planet.global> wrote:
>
> On 07/10/2022 13:45, Jørgen Kvalsvik wrote:
> > On 07/10/2022 08:53, Richard Biener wrote:
> >> On Thu, Oct 6, 2022 at 4:28 PM Jørgen Kvalsvik
> >> <jorgen.kvalsvik@woven-planet.global> wrote:
> >>>
> >>> On 06/10/2022 10:12, Richard Biener wrote:
> >>>> On Wed, Oct 5, 2022 at 2:49 PM Martin Liška <mliska@suse.cz> wrote:
> >>>>>
> >>>>> 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.  */
> >>>>
> >>>> The comment no longer matches the code.>
> >>>>>> -           for (gsi = gsi_last_nondebug_bb (bb);
> >>>>>> +           for (gsi = gsi_start_nondebug_bb (bb);
> >>>>
> >>>> ^^^ and you are looking at the branch block stmts (bb), not the destination
> >>>> block stmts (e->dest)
> >>>>
> >>>>>>                  !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)))))
> >>>>
> >>>> this heuristic needs to be in sync with how we insert edge counters
> >>>> which seems to be hidden in the MST compute (and/or edge insertion).
> >>>> I don't see how it's a win to disregard 'last' and only consider 'dest' here.
> >>>>
> >>>> I think the patch is wrong.  Please revert if you applied it already.
> >>>
> >>> I haven't applied it yet, so unless someone beat me to it there's fortunately
> >>> nothing to revert.
> >>>
> >>>> I don't see how it's a win to disregard 'last' and only consider 'dest' here.
> >>>
> >>> It might not be other than that it unbreaks my condition profiling by not
> >>> splitting condition edges and apparently doesn't cause a regression (one caught
> >>> by tests anyway). That being said the heuristic may very well be wrong (as is
> >>> the implementation since it considers bb and not e->dest, although I'm sure I
> >>> tested it with e->dest at some point).
> >>>
> >>> I guess the most important question is if the if (a && b) {} {label:} edges
> >>> should be split in the first place. As mentioned in the patch set, the only
> >>> difference in the test suite happens on break in switches. I'll tinker a bit
> >>> more to see if I can figure out what's going on or if the heuristic can
> >>> otherwise be improved.
> >>>
> >>> Then, when does a block with a goto_locus edge have multiple successors? From my
> >>> previous testing it doesn't seem like it's the conditions make a goto_locus, but
> >>> it's more than just plain gotos right? When would it then have multiple
> >>> successors? Switches and exception handling? If that's the case then maybe the
> >>> heuristic can be improved by simply checking the edge type.
> >>
> >> The goto_locus of an edge is usually either the locus of the control stmt or the
> >> locus of the stmt the control transfers to.  The most important case is for
> >> 'goto' stmts themselves since those are elided and become edges (thus
> >> 'goto_locus').
> >>
> >> My understanding as of why we split edges at all is that we want to instrument
> >> different locations with different counters and since we cannot have counters on
> >> edges itself but have to either insert the counter on the source or
> >> the destination
> >> we in some cases have to split the edge to create an insert location
> >> to not falsely
> >> account.  instrument_edges seems to simply use gsi_insert_on_edge which
> >> inserts with the gimple_find_edge_insert_loc logic which doesn't look
> >> at goto_locus
> >> at all.  So the existing heuristic must be fragile as well.
> >>
> >> BUT - as you say, the plain goto shouldn't be subject to edge instrumentation.
> >> The interesting case is probably computed goto (which has multiple successors)
> >> and from what I can see a branch where we take the goto_locus from the
> >> COND_EXPRs then/else goto stmt which in theory could have different locations.
> >>
> >> I don't fully understand your requirement of not splitting edges -
> >> I'll just note that
> >> the place you are patching is not the only place where edges are split (but
> >> the insert location computation only sees those splits).
> >>
> >> Richard.
> >
> > Ok, I think I understand. To move forward I propose this additional test case,
> > if anything to catch regressions. Naturally, it fails when the split does not
> > happen because the 'first' label gets incremented twice (I'll probably rename it
> > pre application, assuming it gets approved) not once.
> >
> > This also means I need to change strategy for condition coverage - either, I
> > must snapshot these splits and make a mapping table for the "original"
> > identities or maybe run the analysis before this splitting happens.
> >
> >
> > diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c
> > b/gcc/testsuite/gcc.misc-tests/gcov-4.c
> > index 498d299b66b..b1dc29b573a 100644
> > --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
> > +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
> > @@ -110,6 +110,29 @@ lab2:
> >    return 8;                          /* count(1) */
> >  }
> >
> > +int
> > +test_goto3 (int i, int j)
> > +{
> > +    if (j) goto first;               /* count(1) */
> > +
> > +top:
> > +    if (i)                   /* count(1) */
> > +      {
> > +     i = do_something (i);
> > +      }
> > +    else
> > +      {
> > +first:                               /* count(1) */
> > +     j = do_something (j);   /* count(2) */
> > +     if (j)                  /* count(2) */
> > +       {
> > +         j = 0;              /* count(1) */
> > +         goto top;           /* count(1) */
> > +       }
> > +      }
> > +    return 16;
> > +}
> > +
> >  void
> >  call_goto ()
> >  {
> > @@ -117,6 +140,7 @@ call_goto ()
> >    goto_val += test_goto1 (1);
> >    goto_val += test_goto2 (3);
> >    goto_val += test_goto2 (30);
> > +  goto_val += test_goto3 (0, 1);
> >  }
> >
> >  /* Check nested if-then-else statements. */
> > @@ -260,7 +284,7 @@ main()
> >    call_unref ();
> >    if ((for_val1 != 12)
> >        || (for_val2 != 87)
> > -      || (goto_val != 15)
> > +      || (goto_val != 31)
> >        || (ifelse_val1 != 31)
> >        || (ifelse_val2 != 23)
> >        || (ifelse_val3 != 246)
> >
> > What do you think?
> >
> > Thanks,
> > Jørgen
>
> Hello,
>
> After tinkering a bit more I figured out a patch I could do to merge these
> splits in the condition coverage code, rather than relying on the splits not
> happening. I still think the tests are a good addition, but there's no longer a
> reason for me to change the splitting heuristics.
>
> Should I prepare a separate patch set for the tests?

Yes, enhancing test coverage is always good and should be done
separately.

Thanks a lot,
Richard.

>
> Thanks,
> Jørgen

[PATCH 1/2] gcov: test switch/break line counts

[Jørgen Kvalsvik]

The coverage support will under some conditions decide to split edges to
accurately report coverage. By running the test suite with/without this
edge splitting a small diff shows up, addressed by this patch, which
should catch future regressions.

Removing the edge splitting:

    $ diff --git a/gcc/profile.cc b/gcc/profile.cc
    --- a/gcc/profile.cc
    +++ b/gcc/profile.cc
    @@ -1244,19 +1244,7 @@ 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)
    -                 && !RESERVED_LOCATION_P (e->goto_locus)
    -                 && !single_succ_p (bb)
    -                 && (LOCATION_FILE (e->goto_locus)
    -                     != LOCATION_FILE (gimple_location (last))
    -                     || (LOCATION_LINE (e->goto_locus)
    -                         != LOCATION_LINE (gimple_location (last)))))
    -               {
    -                 basic_block new_bb = split_edge (e);
    -                 edge ne = single_succ_edge (new_bb);
    -                 ne->goto_locus = e->goto_locus;
    -               }
    +
            if ((e->flags & (EDGE_ABNORMAL | EDGE_ABNORMAL_CALL))
                    && e->dest != EXIT_BLOCK_PTR_FOR_FN (cfun))
                    need_exit_edge = 1;

Assuming the .gcov files from make chec-gcc RUNTESTFLAGS=gcov.exp are
kept:

    $ diff -r no-split-edge with-split-edge | grep -C 2 -E "^[<>]\s\s"
    diff -r sans-split-edge/gcc/gcov-4.c.gcov with-split-edge/gcc/gcov-4.c.gcov
    228c228
    <         -:  224:        break;
    ---
    >         1:  224:        break;
    231c231
    <         -:  227:        break;
    ---
    >     #####:  227:        break;
    237c237
    <         -:  233:        break;
    ---
    >         2:  233:        break;

gcc/testsuite/ChangeLog:

	* g++.dg/gcov/gcov-1.C: Add line count check.
	* gcc.misc-tests/gcov-4.c: Likewise.
---
 gcc/testsuite/g++.dg/gcov/gcov-1.C    | 8 ++++----
 gcc/testsuite/gcc.misc-tests/gcov-4.c | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
index 9018b9a3a73..ee383b480a8 100644
--- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
+++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
@@ -257,20 +257,20 @@ test_switch (int i, int j)
   switch (i)				/* count(5) */
 					/* branch(end) */
     {
-      case 1:
+      case 1:				/* count(1) */
         result = do_something (2);	/* count(1) */
-        break;
+        break;				/* count(1) */
       case 2:
         result = do_something (1024);
         break;
-      case 3:
+      case 3:				/* count(3) */
       case 4:
 					/* branch(67) */
         if (j == 2)			/* count(3) */
 					/* branch(end) */
           return do_something (4);	/* count(1) */
         result = do_something (8);	/* count(2) */
-        break;
+        break;				/* count(2) */
       default:
 	result = do_something (32);	/* count(1) */
 	switch_m++;			/* count(1) */
diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c
index 9d8ab1c1097..498d299b66b 100644
--- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
+++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
@@ -221,7 +221,7 @@ test_switch (int i, int j)
     {
       case 1:
         result = do_something (2);	/* count(1) */
-        break;
+        break;				/* count(1) */
       case 2:
         result = do_something (1024);
         break;
@@ -230,7 +230,7 @@ test_switch (int i, int j)
         if (j == 2)			/* count(3) */
           return do_something (4);	/* count(1) */
         result = do_something (8);	/* count(2) */
-        break;
+        break;				/* count(2) */
       default:
 	result = do_something (32);	/* count(1) */
 	switch_m++;			/* count(1) */
-- 
2.34.0

Re: [PATCH 1/2] gcov: test switch/break line counts

[Michael Matz]

[-- Attachment #1: Type: text/plain, Size: 1535 bytes --]

Hello,

On Tue, 11 Oct 2022, Jørgen Kvalsvik via Gcc-patches wrote:

> The coverage support will under some conditions decide to split edges to
> accurately report coverage. By running the test suite with/without this
> edge splitting a small diff shows up, addressed by this patch, which
> should catch future regressions.
> 
> Removing the edge splitting:
> 
>     $ diff --git a/gcc/profile.cc b/gcc/profile.cc
>     --- a/gcc/profile.cc
>     +++ b/gcc/profile.cc
>     @@ -1244,19 +1244,7 @@ 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)
>     -                 && !RESERVED_LOCATION_P (e->goto_locus)
>     -                 && !single_succ_p (bb)
>     -                 && (LOCATION_FILE (e->goto_locus)
>     -                     != LOCATION_FILE (gimple_location (last))
>     -                     || (LOCATION_LINE (e->goto_locus)
>     -                         != LOCATION_LINE (gimple_location (last)))))
>     -               {
>     -                 basic_block new_bb = split_edge (e);
>     -                 edge ne = single_succ_edge (new_bb);
>     -                 ne->goto_locus = e->goto_locus;
>     -               }
>     +

Assuming this is correct (I really can't say) then the comment needs 
adjustments.  It specifically talks about this very code you remove.


Ciao,
Michael.

Re: [PATCH 1/2] gcov: test switch/break line counts

[Jørgen Kvalsvik]

On 11/10/2022 15:55, Michael Matz wrote:
> Hello,
> 
> On Tue, 11 Oct 2022, Jørgen Kvalsvik via Gcc-patches wrote:
> 
>> The coverage support will under some conditions decide to split edges to
>> accurately report coverage. By running the test suite with/without this
>> edge splitting a small diff shows up, addressed by this patch, which
>> should catch future regressions.
>>
>> Removing the edge splitting:
>>
>>     $ diff --git a/gcc/profile.cc b/gcc/profile.cc
>>     --- a/gcc/profile.cc
>>     +++ b/gcc/profile.cc
>>     @@ -1244,19 +1244,7 @@ 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)
>>     -                 && !RESERVED_LOCATION_P (e->goto_locus)
>>     -                 && !single_succ_p (bb)
>>     -                 && (LOCATION_FILE (e->goto_locus)
>>     -                     != LOCATION_FILE (gimple_location (last))
>>     -                     || (LOCATION_LINE (e->goto_locus)
>>     -                         != LOCATION_LINE (gimple_location (last)))))
>>     -               {
>>     -                 basic_block new_bb = split_edge (e);
>>     -                 edge ne = single_succ_edge (new_bb);
>>     -                 ne->goto_locus = e->goto_locus;
>>     -               }
>>     +
> 
> Assuming this is correct (I really can't say) then the comment needs 
> adjustments.  It specifically talks about this very code you remove.
> 
> 
> Ciao,
> Michael.

Michael,

I apologise for the confusion. The diff there is not a part of the change itself
(note the indentation) but rather a way to reproduce, or at least understand,
the type of change that would trigger the new test error. If it is too confusing
I can re-write the commit message.

Thanks,
Jørgen

Re: [PATCH 1/2] gcov: test switch/break line counts

[Michael Matz]

[-- Attachment #1: Type: text/plain, Size: 280 bytes --]

Hello,

On Tue, 11 Oct 2022, Jørgen Kvalsvik wrote:

> I apologise for the confusion. The diff there is not a part of the 
> change itself (note the indentation) but rather a way to reproduce,

Ah!  Thanks, that explains it, sorry for adding confusion on top :-)


Ciao,
Michael.

Re: [PATCH 1/2] gcov: test switch/break line counts

[Richard Biener]

On Tue, Oct 11, 2022 at 2:43 PM Jørgen Kvalsvik
<jorgen.kvalsvik@woven-planet.global> wrote:
>
> The coverage support will under some conditions decide to split edges to
> accurately report coverage. By running the test suite with/without this
> edge splitting a small diff shows up, addressed by this patch, which
> should catch future regressions.
>
> Removing the edge splitting:
>
>     $ diff --git a/gcc/profile.cc b/gcc/profile.cc
>     --- a/gcc/profile.cc
>     +++ b/gcc/profile.cc
>     @@ -1244,19 +1244,7 @@ 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)
>     -                 && !RESERVED_LOCATION_P (e->goto_locus)
>     -                 && !single_succ_p (bb)
>     -                 && (LOCATION_FILE (e->goto_locus)
>     -                     != LOCATION_FILE (gimple_location (last))
>     -                     || (LOCATION_LINE (e->goto_locus)
>     -                         != LOCATION_LINE (gimple_location (last)))))
>     -               {
>     -                 basic_block new_bb = split_edge (e);
>     -                 edge ne = single_succ_edge (new_bb);
>     -                 ne->goto_locus = e->goto_locus;
>     -               }
>     +
>             if ((e->flags & (EDGE_ABNORMAL | EDGE_ABNORMAL_CALL))
>                     && e->dest != EXIT_BLOCK_PTR_FOR_FN (cfun))
>                     need_exit_edge = 1;
>
> Assuming the .gcov files from make chec-gcc RUNTESTFLAGS=gcov.exp are
> kept:
>
>     $ diff -r no-split-edge with-split-edge | grep -C 2 -E "^[<>]\s\s"
>     diff -r sans-split-edge/gcc/gcov-4.c.gcov with-split-edge/gcc/gcov-4.c.gcov
>     228c228
>     <         -:  224:        break;
>     ---
>     >         1:  224:        break;
>     231c231
>     <         -:  227:        break;
>     ---
>     >     #####:  227:        break;
>     237c237
>     <         -:  233:        break;
>     ---
>     >         2:  233:        break;
>
> gcc/testsuite/ChangeLog:

OK.

Thanks,
Richard.

>
>         * g++.dg/gcov/gcov-1.C: Add line count check.
>         * gcc.misc-tests/gcov-4.c: Likewise.
> ---
>  gcc/testsuite/g++.dg/gcov/gcov-1.C    | 8 ++++----
>  gcc/testsuite/gcc.misc-tests/gcov-4.c | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
> index 9018b9a3a73..ee383b480a8 100644
> --- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
> +++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
> @@ -257,20 +257,20 @@ test_switch (int i, int j)
>    switch (i)                           /* count(5) */
>                                         /* branch(end) */
>      {
> -      case 1:
> +      case 1:                          /* count(1) */
>          result = do_something (2);     /* count(1) */
> -        break;
> +        break;                         /* count(1) */
>        case 2:
>          result = do_something (1024);
>          break;
> -      case 3:
> +      case 3:                          /* count(3) */
>        case 4:
>                                         /* branch(67) */
>          if (j == 2)                    /* count(3) */
>                                         /* branch(end) */
>            return do_something (4);     /* count(1) */
>          result = do_something (8);     /* count(2) */
> -        break;
> +        break;                         /* count(2) */
>        default:
>         result = do_something (32);     /* count(1) */
>         switch_m++;                     /* count(1) */
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c
> index 9d8ab1c1097..498d299b66b 100644
> --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
> @@ -221,7 +221,7 @@ test_switch (int i, int j)
>      {
>        case 1:
>          result = do_something (2);     /* count(1) */
> -        break;
> +        break;                         /* count(1) */
>        case 2:
>          result = do_something (1024);
>          break;
> @@ -230,7 +230,7 @@ test_switch (int i, int j)
>          if (j == 2)                    /* count(3) */
>            return do_something (4);     /* count(1) */
>          result = do_something (8);     /* count(2) */
> -        break;
> +        break;                         /* count(2) */
>        default:
>         result = do_something (32);     /* count(1) */
>         switch_m++;                     /* count(1) */
> --
> 2.34.0
>

Re: [PATCH 1/2] gcov: test switch/break line counts

[Jørgen Kvalsvik]

On 13/10/2022 13:39, Richard Biener wrote:
> On Tue, Oct 11, 2022 at 2:43 PM Jørgen Kvalsvik
> <jorgen.kvalsvik@woven-planet.global> wrote:
>>
>> The coverage support will under some conditions decide to split edges to
>> accurately report coverage. By running the test suite with/without this
>> edge splitting a small diff shows up, addressed by this patch, which
>> should catch future regressions.
>>
>> Removing the edge splitting:
>>
>>     $ diff --git a/gcc/profile.cc b/gcc/profile.cc
>>     --- a/gcc/profile.cc
>>     +++ b/gcc/profile.cc
>>     @@ -1244,19 +1244,7 @@ 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)
>>     -                 && !RESERVED_LOCATION_P (e->goto_locus)
>>     -                 && !single_succ_p (bb)
>>     -                 && (LOCATION_FILE (e->goto_locus)
>>     -                     != LOCATION_FILE (gimple_location (last))
>>     -                     || (LOCATION_LINE (e->goto_locus)
>>     -                         != LOCATION_LINE (gimple_location (last)))))
>>     -               {
>>     -                 basic_block new_bb = split_edge (e);
>>     -                 edge ne = single_succ_edge (new_bb);
>>     -                 ne->goto_locus = e->goto_locus;
>>     -               }
>>     +
>>             if ((e->flags & (EDGE_ABNORMAL | EDGE_ABNORMAL_CALL))
>>                     && e->dest != EXIT_BLOCK_PTR_FOR_FN (cfun))
>>                     need_exit_edge = 1;
>>
>> Assuming the .gcov files from make chec-gcc RUNTESTFLAGS=gcov.exp are
>> kept:
>>
>>     $ diff -r no-split-edge with-split-edge | grep -C 2 -E "^[<>]\s\s"
>>     diff -r sans-split-edge/gcc/gcov-4.c.gcov with-split-edge/gcc/gcov-4.c.gcov
>>     228c228
>>     <         -:  224:        break;
>>     ---
>>     >         1:  224:        break;
>>     231c231
>>     <         -:  227:        break;
>>     ---
>>     >     #####:  227:        break;
>>     237c237
>>     <         -:  233:        break;
>>     ---
>>     >         2:  233:        break;
>>
>> gcc/testsuite/ChangeLog:
> 
> OK.
> 
> Thanks,
> Richard.
> 
>>
>>         * g++.dg/gcov/gcov-1.C: Add line count check.
>>         * gcc.misc-tests/gcov-4.c: Likewise.
>> ---
>>  gcc/testsuite/g++.dg/gcov/gcov-1.C    | 8 ++++----
>>  gcc/testsuite/gcc.misc-tests/gcov-4.c | 4 ++--
>>  2 files changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C b/gcc/testsuite/g++.dg/gcov/gcov-1.C
>> index 9018b9a3a73..ee383b480a8 100644
>> --- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
>> +++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
>> @@ -257,20 +257,20 @@ test_switch (int i, int j)
>>    switch (i)                           /* count(5) */
>>                                         /* branch(end) */
>>      {
>> -      case 1:
>> +      case 1:                          /* count(1) */
>>          result = do_something (2);     /* count(1) */
>> -        break;
>> +        break;                         /* count(1) */
>>        case 2:
>>          result = do_something (1024);
>>          break;
>> -      case 3:
>> +      case 3:                          /* count(3) */
>>        case 4:
>>                                         /* branch(67) */
>>          if (j == 2)                    /* count(3) */
>>                                         /* branch(end) */
>>            return do_something (4);     /* count(1) */
>>          result = do_something (8);     /* count(2) */
>> -        break;
>> +        break;                         /* count(2) */
>>        default:
>>         result = do_something (32);     /* count(1) */
>>         switch_m++;                     /* count(1) */
>> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c
>> index 9d8ab1c1097..498d299b66b 100644
>> --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c
>> +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c
>> @@ -221,7 +221,7 @@ test_switch (int i, int j)
>>      {
>>        case 1:
>>          result = do_something (2);     /* count(1) */
>> -        break;
>> +        break;                         /* count(1) */
>>        case 2:
>>          result = do_something (1024);
>>          break;
>> @@ -230,7 +230,7 @@ test_switch (int i, int j)
>>          if (j == 2)                    /* count(3) */
>>            return do_something (4);     /* count(1) */
>>          result = do_something (8);     /* count(2) */
>> -        break;
>> +        break;                         /* count(2) */
>>        default:
>>         result = do_something (32);     /* count(1) */
>>         switch_m++;                     /* count(1) */
>> --
>> 2.34.0
>>

Thank you, I've installed the patches.

I noticed that the inclusion of diffs in the message works fine with git
generally, but could be picked up on by git format-patch && git am < patch. I
hope that does not end up causing too many problems (last time, promise!)

Thanks,
Jørgen