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?
Thanks,
Xionghu
