On Wed, 8 Mar 2023, Xionghu Luo wrote:
>
>
> On 2023/3/7 19:25, Richard Biener wrote:
> >>> 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.
>
> Tried to add function compute_target_labels(not sure whether the function
> name is suitable) in the front of make_blocks_1, now the fortran case doesn't
> create/removing blocks now, but I still have several questions:
>
> 1. I used hash_set<tree> to save the target labels instead of bitmap, as
> labels
> are tree type value instead of block index so bitmap is not good for it since
> we don't have LABEL_DECL_UID now?
We don't have LABEL_DECL_UID, we have DECL_UID though, but the choice of
hash_set<tree> vs. bitmap is somewhat arbitrary here. The real cost is
the extra walk over all stmts.
> 2. Is the compute_target_labels still only for !optimize? And if we compute
> the target labels before create bbs, it is unnessary to guard the first
> cleanup_dead_labels under !optimize now, because the switch-case-do-while
> case already create new block for CASE_LABEL already.
OK.
> 3. I only added GIMPLE_SWITCH/GIMPLE_COND in compute_target_labels
> so far, is it needed to also handle GIMPLE_ASM/GIMPLE_TRANSACTION and even
> labels_eh?
I'd add GIMPLE_ASM handling, the rest should be OK wrt debugging and
coverage already?
> PS1: The v3 patch will cause one test case fail:
>
> Number of regressions in total: 1
> > FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess
> > errors)
>
> due to this exausting case has labels from L0 to L100001, they won't be
> optimized
> to a simple if-else expression like before...
Hmm, that's somewhat unexpected.
>
> PS2: The GIMPLE_GOTO piece of code would cause some fortran cases run fail due
> to __builtin_unreachable trap generated in .fixup_cfg1, I didn't dig into it
> so
> just skip these label...
Please investigate, we might be missing a corner case here.
>
> + case GIMPLE_GOTO:
> +#if 0
> + if (!computed_goto_p (stmt))
> + {
> + tree dest = gimple_goto_dest (stmt);
> + target_labels->add (dest);
> + }
> +#endif
> + break;
>
> Change the #if 0 to #if 1 result in:
>
> Number of regressions in total: 8
> > FAIL: gcc.c-torture/compile/limits-caselabels.c -O0 (test for excess
> > FAIL: errors)
> > FAIL: gcc.dg/analyzer/explode-2a.c (test for excess errors)
> > FAIL: gcc.dg/analyzer/pragma-2.c (test for excess errors)
> > FAIL: gfortran.dg/bound_2.f90 -O0 execution test
> > FAIL: gfortran.dg/bound_7.f90 -O0 execution test
> > FAIL: gfortran.dg/char_result_14.f90 -O0 execution test
> > FAIL: gfortran.dg/pointer_array_1.f90 -O0 execution test
> > FAIL: gfortran.dg/select_type_15.f03 -O0 execution test
>
>
>
> Paste the updated patch v3:
The gcov testcase adjustments look good, does the analyzer testcase
(missing in the changelog) get different CFG input?
Thanks,
Richard.
>
> v3: Add compute_target_labels and call it in the front of make_blocks_1.
>
> Start a new basic block if two labels have different location when
> test-coverage.
>
> Regression tested pass on x86_64-linux-gnu and aarch64-linux-gnu, OK for
> master?
>
> gcc/ChangeLog:
>
> PR gcov/93680
> * tree-cfg.cc (stmt_starts_bb_p): Check whether the label is in
> target_labels.
> (compute_target_labels): New function.
> (make_blocks_1): Call compute_target_labels.
>
> gcc/testsuite/ChangeLog:
>
> PR gcov/93680
> * g++.dg/gcov/gcov-1.C: Correct counts.
> * gcc.misc-tests/gcov-4.c: Likewise.
> * gcc.misc-tests/gcov-pr85332.c: Likewise.
> * lib/gcov.exp: Also clean gcda if fail.
> * gcc.misc-tests/gcov-pr93680.c: New test.
>
> Signed-off-by: Xionghu Luo <xionghu...@tencent.com>
> ---
> gcc/tree-cfg.cc | 68 ++++++++++++++++++++-
> gcc/testsuite/g++.dg/gcov/gcov-1.C | 2 +-
> gcc/testsuite/gcc.dg/analyzer/paths-4.c | 8 +--
> gcc/testsuite/gcc.misc-tests/gcov-pr85332.c | 2 +-
> gcc/testsuite/gcc.misc-tests/gcov-pr93680.c | 24 ++++++++
> gcc/testsuite/lib/gcov.exp | 4 +-
> 6 files changed, 96 insertions(+), 12 deletions(-)
> create mode 100644 gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
>
> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
> index a9fcc7fd050..0f8efcf4aa3 100644
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> @@ -164,7 +164,7 @@ static edge gimple_redirect_edge_and_branch (edge,
> basic_block);
> static edge gimple_try_redirect_by_replacing_jump (edge, basic_block);
>
> /* Various helpers. */
> -static inline bool stmt_starts_bb_p (gimple *, gimple *);
> +static inline bool stmt_starts_bb_p (gimple *, gimple *, hash_set<tree> *);
> static int gimple_verify_flow_info (void);
> static void gimple_make_forwarder_block (edge);
> static gimple *first_non_label_stmt (basic_block);
> @@ -521,6 +521,59 @@ gimple_call_initialize_ctrl_altering (gimple *stmt)
> gimple_call_set_ctrl_altering (stmt, false);
> }
>
> +/* Compute target labels to save useful labels. */
> +static void
> +compute_target_labels (gimple_seq seq, hash_set<tree> *target_labels)
> +{
> + gimple *stmt = NULL;
> + gimple_stmt_iterator j = gsi_start (seq);
> +
> + while (!gsi_end_p (j))
> + {
> + stmt = gsi_stmt (j);
> +
> + switch (gimple_code (stmt))
> + {
> + case GIMPLE_COND:
> + {
> + gcond *cstmt = as_a <gcond *> (stmt);
> + tree true_label = gimple_cond_true_label (cstmt);
> + tree false_label = gimple_cond_false_label (cstmt);
> + target_labels->add (true_label);
> + target_labels->add (false_label);
> + }
> + break;
> + case GIMPLE_SWITCH:
> + {
> + gswitch *gstmt = as_a <gswitch *> (stmt);
> + size_t i, n = gimple_switch_num_labels (gstmt);
> + tree elt, label;
> + for (i = 0; i < n; i++)
> + {
> + elt = gimple_switch_label (gstmt, i);
> + label = CASE_LABEL (elt);
> + target_labels->add (label);
> + }
> + }
> + break;
> + case GIMPLE_GOTO:
> +#if 0
> + if (!computed_goto_p (stmt))
> + {
> + tree dest = gimple_goto_dest (stmt);
> + target_labels->add (dest);
> + }
> +#endif
> + break;
> +
> + default:
> + break;
> + }
> +
> + gsi_next (&j);
> + }
> +}
> +
>
> /* Insert SEQ after BB and build a flowgraph. */
>
> @@ -532,6 +585,10 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
> gimple *prev_stmt = NULL;
> bool start_new_block = true;
> bool first_stmt_of_seq = true;
> + hash_set<tree> target_labels;
> +
> + if (!optimize)
> + compute_target_labels (seq, &target_labels);
>
> while (!gsi_end_p (i))
> {
> @@ -553,7 +610,7 @@ make_blocks_1 (gimple_seq seq, basic_block bb)
> /* If the statement starts a new basic block or if we have determined
> in a previous pass that we need to create a new block for STMT, do
> so now. */
> - if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt))
> + if (start_new_block || stmt_starts_bb_p (stmt, prev_stmt,
> &target_labels))
> {
> if (!first_stmt_of_seq)
> gsi_split_seq_before (&i, &seq);
> @@ -2832,7 +2889,8 @@ simple_goto_p (gimple *t)
> label. */
>
> static inline bool
> -stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
> +stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt,
> + hash_set<tree> *target_labels)
> {
> if (stmt == NULL)
> return false;
> @@ -2860,6 +2918,10 @@ stmt_starts_bb_p (gimple *stmt, gimple *prev_stmt)
> || !DECL_ARTIFICIAL (gimple_label_label (plabel)))
> return true;
> + if (!optimize
> + && target_labels->contains (gimple_label_label (label_stmt)))
> + return true;
> +
> cfg_stats.num_merged_labels++;
> return false;
> }
> diff --git a/gcc/testsuite/g++.dg/gcov/gcov-1.C
> b/gcc/testsuite/g++.dg/gcov/gcov-1.C
> index ee383b480a8..01e7084fb03 100644
> --- a/gcc/testsuite/g++.dg/gcov/gcov-1.C
> +++ b/gcc/testsuite/g++.dg/gcov/gcov-1.C
> @@ -263,7 +263,7 @@ test_switch (int i, int j)
> case 2:
> result = do_something (1024);
> break;
> - case 3: /* count(3) */
> + case 3: /* count(2) */
> case 4:
> /* branch(67) */
> if (j == 2) /* count(3) */
> diff --git a/gcc/testsuite/gcc.dg/analyzer/paths-4.c
> b/gcc/testsuite/gcc.dg/analyzer/paths-4.c
> index b72e658739e..fdf33e68d0c 100644
> --- a/gcc/testsuite/gcc.dg/analyzer/paths-4.c
> +++ b/gcc/testsuite/gcc.dg/analyzer/paths-4.c
> @@ -35,18 +35,18 @@ int test_2 (struct state *s)
> do_stuff (s, 0);
> break;
> case 1:
> - __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed
> enode" } */
> + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed
> enode" } */
> do_stuff (s, 17);
> break;
> case 2:
> - __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed
> enode" } */
> + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed
> enode" } */
> do_stuff (s, 5);
> break;
> case 3:
> - __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed
> enode" } */
> + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed
> enode" } */
> return 42;
> case 4:
> - __analyzer_dump_exploded_nodes (0); /* { dg-warning "1 processed
> enode" } */
> + __analyzer_dump_exploded_nodes (0); /* { dg-warning "2 processed
> enode" } */
> return -3;
> }
> }
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
> b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
> index 73e50b19fc7..b37e760910c 100644
> --- a/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr85332.c
> @@ -7,7 +7,7 @@ int doit(int sel, int n, void *p)
>
> switch (sel)
> {
> - case 0: /* count(3) */
> + case 0: /* count(1) */
> do {*p0 += *p0;} while (--n); /* count(3) */
> return *p0 == 0; /* count(1) */
> diff --git a/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> new file mode 100644
> index 00000000000..2fe340c4011
> --- /dev/null
> +++ b/gcc/testsuite/gcc.misc-tests/gcov-pr93680.c
> @@ -0,0 +1,24 @@
> +/* { dg-options "-fprofile-arcs -ftest-coverage" } */
> +/* { dg-do run { target native } } */
> +
> +int f(int s, int n)
> +{
> + int p = 0;
> +
> + switch (s)
> + {
> + case 0: /* count(1) */
> + do { p++; } while (--n); /* count(5) */
> + return p; /* count(1) */
> +
> + case 1: /* count(1) */
> + do { p++; } while (--n); /* count(5) */
> + return p; /* count(1) */
> + }
> +
> + return 0;
> +}
> +
> +int main() { f(0, 5); f(1, 5); return 0; }
> +
> +/* { dg-final { run-gcov gcov-pr93680.c } } */
> diff --git a/gcc/testsuite/lib/gcov.exp b/gcc/testsuite/lib/gcov.exp
> index 80e74aeb220..07e1978d25d 100644
> --- a/gcc/testsuite/lib/gcov.exp
> +++ b/gcc/testsuite/lib/gcov.exp
> @@ -424,9 +424,7 @@ proc run-gcov { args } {
> }
> if { $tfailed > 0 } {
> fail "$testname gcov: $lfailed failures in line counts, $bfailed in
> branch percentages, $cfailed in return percentages, $ifailed in intermediate
> format"
> - if { $xfailed } {
> - clean-gcov $testcase
> - }
> + clean-gcov $testcase
> } else {
> pass "$testname gcov"
> clean-gcov $testcase
>
--
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)
