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

[Richard Biener <richard.guenther@gmail.com>]

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