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

["Jørgen Kvalsvik" <jorgen.kvalsvik@woven-planet.global>]

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