On Tue, 7 Mar 2023, Xionghu Luo wrote:
>
>
> On 2023/3/7 16:53, Richard Biener wrote:
> > On Tue, 7 Mar 2023, Xionghu Luo wrote:
>
> >> 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;
>
> It was:
>
> <bb 0> :
>
> <bb 2> :
> L.1:
>
> <bb 3> :
> L.2:
>
> <bb 4> :
> L.3:
>
> <bb 5> :
> L.4:
>
> <bb 6> :
> L.5:
>
> <bb 7> :
> L.6:
> return;
>
> <bb 1> :
>
> before the second call of cleanup_dead_labels, after it, all labels are
> removed, then tree_forwarder_block_p remove all forworders. Yes, it
> creates blocks and remove blocks immediately...
>
> >
> > 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.
>
> Yes. Should also exclude this.
>
> >
> > 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.
>
> For the backedge case with switch-case-do-while, tree_forwarder_block_p
> returns false when iterating the statement check.
> The new created <bb 3> with only one case label instruction still owns
> location information in it, so CFG cleanup won't remove the forwarders.
>
> 390│ for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
> 391│ {
> 392│ gimple *stmt = gsi_stmt (gsi);
> 393│
> 394│ switch (gimple_code (stmt))
> 395│ {
> 396│ case GIMPLE_LABEL:
> 397│ if (DECL_NONLOCAL (gimple_label_label (as_a <glabel *>(stmt))))
> 398│ return false;
> 399│ if (!optimize
> 400│ && (gimple_has_location (stmt)
> 401│ || LOCATION_LOCUS (locus) != UNKNOWN_LOCATION)
> 402│ && gimple_location (stmt) != locus)
> 403├> return false;
> 404│ break;
>
>
> (gdb) ps stmt
> <L0>:
> (gdb) p gimple_location (stmt)
> $154 = 2147483656
> (gdb) pel $154
> {file = 0x3e41af0 "small.c", line = 7, column = 5, data = 0x7ffff6f80420, sysp
> = false}
> (gdb)
> (gdb) pbb bb
> ;; basic block 3, loop depth 0
> ;; pred: 2
> <L0>:
> ;; succ: 4
>
> >
> > 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.
> >
>
>
> <bb 2> :
> p = 0;
> switch (s) <default: <D.2756>, case 0: <L0>, case 1: <D.2744>>
>
> <bb 3> :
> <L0>: <= prev_stmt
> <D.2748>: <= stmt
> p = p + 1;
> n = n + -1;
> if (n != 0) goto <D.2748>; else goto <D.2746>;
>
> Check if <L0> is a case label and <D.2748> is a goto target then return true
> in stmt_starts_bb_p to start a new basic block? This would avoid creating and
> removing blocks, but cleanup_dead_labels has all bbs setup while
> stmt_starts_bb_p
> does't yet to iterate bbs/labels to establish label_for_bb[] map?
Yes. I think we'd need something more pragmatic before make_blocks (),
like re-computing TREE_USED of the label decls or computing a bitmap
of targeted labels (targeted by goto, switch or any other means).
I'll note that doing a cleanup_dead_labels () like optimization before
we create blocks will help keeping LABEL_DECL_UID and thus
label_to_block_map dense. But it does look like a bit of
an chicken-and-egg problem and the question is how effective the
dead label removal is in practice.
Richard.
