On Tue, 7 Mar 2023, Xionghu Luo wrote:
>
>
> On 2023/3/6 16:11, Richard Biener wrote:
> > On Mon, Mar 6, 2023 at 8:22 AM Xionghu Luo <yinyuefen...@gmail.com> wrote:
> >>
> >>
> >>
> >> On 2023/3/2 18:45, Richard Biener wrote:
> >>>>
> >>>>
> >>>> small.gcno: 648: block 2:`small.c':1, 3, 4, 6
> >>>> small.gcno: 688: 01450000: 36:LINES
> >>>> small.gcno: 700: block 3:`small.c':8, 9
> >>>> small.gcno: 732: 01450000: 32:LINES
> >>>> small.gcno: 744: block 5:`small.c':10
> >>>> -small.gcno: 772: 01450000: 32:LINES
> >>>> -small.gcno: 784: block 6:`small.c':12
> >>>> -small.gcno: 812: 01450000: 36:LINES
> >>>> -small.gcno: 824: block 7:`small.c':12, 13
> >>>> +small.gcno: 772: 01450000: 36:LINES
> >>>> +small.gcno: 784: block 6:`small.c':12, 13
> >>>> +small.gcno: 816: 01450000: 32:LINES
> >>>> +small.gcno: 828: block 8:`small.c':14
> >>>> small.gcno: 856: 01450000: 32:LINES
> >>>> -small.gcno: 868: block 8:`small.c':14
> >>>> -small.gcno: 896: 01450000: 32:LINES
> >>>> -small.gcno: 908: block 9:`small.c':17
> >>>> +small.gcno: 868: block 9:`small.c':17
> >>>
> >>> Looking at the CFG and the instrumentation shows
> >>>
> >>> <bb 2> :
> >>> PROF_edge_counter_17 = __gcov0.f[0];
> >>> PROF_edge_counter_18 = PROF_edge_counter_17 + 1;
> >>> __gcov0.f[0] = PROF_edge_counter_18;
> >>> [t.c:3:7] p_6 = 0;
> >>> [t.c:5:3] switch (s_7(D)) <default: <L6> [INV], [t.c:7:5] case 0:
> >>> <L0> [INV], [t.c:11:5] case 1: <L3> [INV]>
> >>>
> >>> <bb 3> :
> >>> # n_1 = PHI <n_8(D)(2), [t.c:8:28] n_13(4)>
> >>> # p_3 = PHI <[t.c:3:7] p_6(2), [t.c:8:15] p_12(4)>
> >>> [t.c:7:5] <L0>:
> >>> [t.c:8:15] p_12 = p_3 + 1;
> >>> [t.c:8:28] n_13 = n_1 + -1;
> >>> [t.c:8:28] if (n_13 != 0)
> >>> goto <bb 4>; [INV]
> >>> else
> >>> goto <bb 5>; [INV]
> >>>
> >>> <bb 4> :
> >>> PROF_edge_counter_21 = __gcov0.f[2];
> >>> PROF_edge_counter_22 = PROF_edge_counter_21 + 1;
> >>> __gcov0.f[2] = PROF_edge_counter_22;
> >>> [t.c:7:5] goto <bb 3>; [100.00%]
> >>>
> >>> <bb 5> :
> >>> PROF_edge_counter_23 = __gcov0.f[3];
> >>> PROF_edge_counter_24 = PROF_edge_counter_23 + 1;
> >>> __gcov0.f[3] = PROF_edge_counter_24;
> >>> [t.c:9:16] _14 = p_12;
> >>> [t.c:9:16] goto <bb 10>; [INV]
> >>>
> >>> so the reason this goes wrong is that gcov associates the "wrong"
> >>> counter with the block containing
> >>> the 'case' label(s), for the case 0 it should have chosen the counter
> >>> from bb5 but it likely
> >>> computed the count of bb3?
> >>>
> >>> It might be that ordering blocks differently puts the instrumentation
> >>> to different blocks or it
> >>> makes gcovs association chose different blocks but that means it's
> >>> just luck and not fixing
> >>> the actual issue?
> >>>
> >>> To me it looks like the correct thing to investigate is switch
> >>> statement and/or case label
> >>> handling. One can also see that <L0> having line number 7 is wrong to
> >>> the extent that
> >>> the position of the label doesn't match the number of times it
> >>> executes in the source. So
> >>> placement of the label is wrong here, possibly caused by CFG cleanup
> >>> after CFG build
> >>> (but generally labels are not used for anything once the CFG is built
> >>> and coverage
> >>> instrumentation is late so it might fail due to us moving labels). It
> >>> might be OK to
> >>> avoid moving labels for --coverage but then coverage should possibly
> >>> look at edges
> >>> rather than labels?
> >>>
> >>
> >> Thanks, I investigated the Labels, it seems wrong at the beginning from
> >> .gimple to .cfg very early quite like PR90574:
> >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90574
> >>
> >> .gimple:
> >>
> >> int f (int s, int n)
> >> [small.c:2:1] {
> >> int D.2755;
> >> int p;
> >>
> >> [small.c:3:7] p = 0;
> >> [small.c:5:3] switch (s) <default: <D.2756>, [small.c:7:5] case 0:
> >> <D.2743>, [small.c:11:5] case 1: <D.2744>>
> >> [small.c:7:5] <D.2743>: <= case label
> >> <D.2748>: <= loop label
> >> [small.c:8:13] p = p + 1;
> >> [small.c:8:26] n = n + -1;
> >> [small.c:8:26] if (n != 0) goto <D.2748>; else goto <D.2746>;
> >> <D.2746>:
> >> [small.c:9:14] D.2755 = p;
> >> [small.c:9:14] return D.2755;
> >> [small.c:11:5] <D.2744>:
> >> <D.2751>:
> >> [small.c:12:13] p = p + 1;
> >> [small.c:12:26] n = n + -1;
> >> [small.c:12:26] if (n != 0) goto <D.2751>; else goto <D.2749>;
> >> <D.2749>:
> >> [small.c:13:14] D.2755 = p;
> >> [small.c:13:14] return D.2755;
> >> <D.2756>:
> >> [small.c:16:10] D.2755 = 0;
> >> [small.c:16:10] return D.2755;
> >> }
> >>
> >> .cfg:
> >>
> >> int f (int s, int n)
> >> {
> >> int p;
> >> int D.2755;
> >>
> >> <bb 2> :
> >> [small.c:3:7] p = 0;
> >> [small.c:5:3] switch (s) <default: <L6> [INV], [small.c:7:5] case 0:
> >> <L0> [INV], [small.c:11:5] case 1: <L3> [INV]>
> >>
> >> <bb 3> :
> >> [small.c:7:5] <L0>: <= case 0
> >> [small.c:8:13 discrim 1] p = p + 1;
> >> [small.c:8:26 discrim 1] n = n + -1;
> >> [small.c:8:26 discrim 1] if (n != 0)
> >> goto <bb 3>; [INV]
> >> else
> >> goto <bb 4>; [INV]
> >>
> >> <bb 4> :
> >> [small.c:9:14] D.2755 = p;
> >> [small.c:9:14] goto <bb 8>; [INV]
> >>
> >> <bb 5> :
> >> [small.c:11:5] <L3>: <= case 1
> >> [small.c:12:13 discrim 1] p = p + 1;
> >> [small.c:12:26 discrim 1] n = n + -1;
> >> [small.c:12:26 discrim 1] if (n != 0)
> >> goto <bb 5>; [INV]
> >> else
> >> goto <bb 6>; [INV]
> >>
> >>
> >> The labels are merged into the loop unexpected, so I tried below fix
> >> for --coverage if two labels are not on same line to start new basic block:
> >>
> >>
> >> index 10ca86714f4..b788198ac31 100644
> >> --- a/gcc/tree-cfg.cc
> >> +++ b/gcc/tree-cfg.cc
> >> @@ -2860,6 +2860,13 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
> >> || !DECL_ARTIFICIAL (gimple_label_label (plabel)))
> >> return true;
> >>
> >> + location_t loc_prev = gimple_location (plabel);
> >> + location_t locus = gimple_location (label_stmt);
> >> + expanded_location locus_e = expand_location (locus);
> >> +
> >> + if (flag_test_coverage && !same_line_p (locus, &locus_e,
> >> loc_prev))
> >> + return true;
> >> +
> >> cfg_stats.num_merged_labels++;
> >> return false;
> >> }
> >
> > Interesting. Note that in CFG cleanup we have the following condition
> > when deciding
> > whether to merge a forwarder block with the destination:
> >
> > locus = single_succ_edge (bb)->goto_locus;
> > ...
> > /* Now walk through the statements backward. We can ignore labels,
> > anything else means this is not a forwarder block. */
> > for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
> > {
> > gimple *stmt = gsi_stmt (gsi);
> >
> > switch (gimple_code (stmt))
> > {
> > case GIMPLE_LABEL:
> > if (DECL_NONLOCAL (gimple_label_label (as_a <glabel *> (stmt))))
> > return false;
> > if (!optimize
> > && (gimple_has_location (stmt)
> > || LOCATION_LOCUS (locus) != UNKNOWN_LOCATION)
> > && gimple_location (stmt) != locus)
> > return false;
> >
> > it would be nice to sync the behavior between CFG creation and this.
> > In particular
> > a missing piece of the puzzle is how CFG creation sets ->goto_locus of the
> > edge
> > after your change and whether that goto_locus and the label locus
> > compare matches
> > your condition (the CFG cleanup one is even stricter but special cases
> > UNKNOWN_LOCATION).
> >
> > I also notice the !optimize vs. flag_test_coverage condition mismatch.
> >
> > That said - I think your change to stmt_starts_bb_p is definitely the
> > correct place to fix,
> > I'm wondering how to match up with CFG cleanup - possibly using
> > !optimize instead
> > of flag_test_coverage would even make sense for debugging as well - we
> > should be
> > able to put a breakpoint on the label hitting once rather than once
> > each loop iteration.
> >
>
> Unfortunately this change (flag_test_coverage -> !optimize ) caused hundred
> of gfortran cases execution failure with O0. Take gfortran.dg/index.f90 for
> example:
>
> .gimple:
>
> __attribute__((fn spec (". ")))
> void p ()
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:6:9] {
>
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:13:28]
> L.1:
>
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:14:28]
> L.2:
>
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:15:28]
> L.3:
>
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:16:28]
> L.4:
>
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:17:28]
> L.5:
>
> [/data/RocksDB_Docker/tgcc-master/gcc/testsuite/gfortran.dg/index_4.f90:18:72]
> L.6:
> }
>
> .cfg:
>
> ...
> Removing basic block 7
> ;; basic block 7, loop depth 0
> ;; pred:
> return;
> ;; succ: EXIT
>
>
> ;; 1 loops found
> ;;
> ;; Loop 0
> ;; header 0, latch 1
> ;; depth 0, outer -1
> ;; nodes: 0 1 2
> ;;2 succs { }
> __attribute__((fn spec (". ")))
> void p ()
> {
> <bb 2> :
>
> }
>
> Due to the "return;" is removed in bb 7.
OK, the issue is that make_edges_bb does nothing for an empty block
but it should at least create a fallthru edge here. Thus,
if (!last)
fallthru = true;
else
switch (gimple_code (last))
{
instead of simply returning if (!last). The alternative would be
to make sure that cleanup_dead_labels preserves at least one
statement in a block.
Looking at the testcases I wonder if preserving all the fallthru labels
is really necessary - for coverage we should have a counter ready. For
the testcase we arrive with
L.1:
L.2:
L.3:
L.4:
i = 1;
where the frontend simplified things but put labels at each line.
I suppose we could optimize this by re-computing TREE_USED and only
splitting before labels reached by a control statement? That would
cover the backedge case in the original testcase. cleanup_dead_labels
does something like that already.
> actually in build_gimple_cfg, cleanup_dead_labels will remove all labels L.1
> to L.6
> first, then make_edges fail to create edges for <bb 2> to <bb 7> due to they
> are all
> EMPTY bb in make_edges_bb...
>
>
> 240│ /* To speed up statement iterator walks, we first purge dead labels.
> */
> 241│ cleanup_dead_labels ();
> 242│
> 243│ /* Group case nodes to reduce the number of edges.
> 244│ We do this after cleaning up dead labels because otherwise we
> miss
> 245│ a lot of obvious case merging opportunities. */
> 246│ group_case_labels ();
> 247│
> 248│ /* Create the edges of the flowgraph. */
> 249│ discriminator_per_locus = new hash_table<locus_discrim_hasher> (13);
> 250├> make_edges ();
>
>
> <bb 0> :
>
> <bb 2> :
>
> <bb 3> :
>
> <bb 4> :
>
> <bb 5> :
>
> <bb 6> :
>
> <bb 7> :
> return;
>
> <bb 1> :
>
>
> Seems deadlock here as you said to set goto_locus as labels are removed before
> edges are created, the case could pass if I comment out the function
> cleanup_dead_labels(),
> so also not call it when !optimize?
>
> if (!!optimize)
> cleanup_dead_labels ();
That probably makes sense. Looking at group_case_labels () that also
seems to do unwanted things (to debugging and coverage), its comment
says that for
switch (i)
{
case 1:
/* fallthru */
case 2:
/* fallthru */
case 3:
k = 0;
it would replace that with
case 1..3:
k = 0;
but that also fails to produce correct coverage, right? Likewise
setting breakpoints.
Does preserving the labels help setting a goto_locus for the
fallthru edges? I don't see any code doing that, so
CFG cleanup will remove the forwarders we created again.
It would be nice to avoid creating blocks / preserving labels we'll
immediately remove again. For that we do need some analysis
before creating basic-blocks that determines whether a label is
possibly reached by a non-falltru edge.
Richard.
>
> Attached v2 patch could pass regression test on
> x86_64-linux-gnu/aarch64-linux-gnu.
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
