On Tue, 10 Mar 2020, Roman Zhuykov wrote:
> Hi!
>
> Current modulo-sched implementation is a bit faulty from -fcompile-debug
> perspective.
>
> I found that few years ago, the most simple example is pr42631.c which fails
> (with just -fmodulo-sched or together with -fmodulo-sched-allow-regmoves) on
> powerpc64le with at least any gcc-4.9 or newer compiler.
> I've investigated that difference about 3 years ago, it is mostly technical,
> dumps shows there are some "flying" NOTE_INSN_DELETED items.
> I understood that it is minor, and I planned to commit the fix only when my
> other modulo-sched stuff will be ready.
>
> But right now I see that when I enable -fmodulo-sched by default, powerpc64le
> bootstrap give comparison failure as of r10-7056.
>
> Comparing stages 2 and 3
> Bootstrap comparison failure!
> gcc/ggc-page.o differs
>
> That doesn't happen on released branches, so it is a kind of "regression"
> (certainly, nobody runs bootstrap with -fmodulo-sched).
>
> Is that a good reason to commit the patch right now in stage4?
Yes from a RM perspective, no comments about the technical details of
the patch.
Richard.
> Patch was successfully regstrapped (based on r10-7056) using x86_64 and
> powerpc64le, both with default options and with -fmodulo-sched enabled.
>
> Roman
>
> --
> modulo-sched: fix compare-debug issues
>
> This patch fixes bootstrap comparison failure on powerpc64le when running it
> with -fmodulo-sched enabled.
>
> When applying the schedule we have to move debug insns in the same
> way as we move note insns. Also we have to discard adding debug insns
> to SCCs in DDG graph.
>
> 20YY-MM-DD Roman Zhuykov <zhr...@ispras.ru>
>
> * ddg.c (create_ddg_dep_from_intra_loop_link): Adjust assertions.
> (create_ddg_dep_no_link): Likewise.
> (add_inter_loop_mem_dep): Do not create "debug --> non-debug" anti-deps.
> (create_ddg): Adjust first_note field filling.
> (check_sccs): Assert if any debug instruction is in SCC.
> * modulo-sched.c (ps_first_note): Add an assertion if first_note
> is empty.
>
> testsuite:
>
> 20YY-MM-DD Roman Zhuykov <zhr...@ispras.ru>
>
> * gcc.dg/pr42631-sms1.c: New test.
> * gcc.dg/pr42631-sms2.c: New test.
>
> diff --git a/gcc/ddg.c b/gcc/ddg.c
> index ca8cb74823..048207a354 100644
> --- a/gcc/ddg.c
> +++ b/gcc/ddg.c
> @@ -185,8 +185,8 @@ create_ddg_dep_from_intra_loop_link (ddg_ptr g,
> ddg_node_ptr src_node,
> else if (DEP_TYPE (link) == REG_DEP_OUTPUT)
> t = OUTPUT_DEP;
>
> - gcc_assert (!DEBUG_INSN_P (dest_node->insn) || t == ANTI_DEP);
> - gcc_assert (!DEBUG_INSN_P (src_node->insn) || t == ANTI_DEP);
> + gcc_assert (NONDEBUG_INSN_P (dest_node->insn) || t == ANTI_DEP);
> + gcc_assert (NONDEBUG_INSN_P (src_node->insn) || DEBUG_INSN_P
> (dest_node->insn));
>
> /* We currently choose not to create certain anti-deps edges and
> compensate for that by generating reg-moves based on the life-range
> @@ -222,9 +222,9 @@ create_ddg_dep_from_intra_loop_link (ddg_ptr g,
> ddg_node_ptr src_node,
> }
> }
>
> - latency = dep_cost (link);
> - e = create_ddg_edge (src_node, dest_node, t, dt, latency, distance);
> - add_edge_to_ddg (g, e);
> + latency = dep_cost (link);
> + e = create_ddg_edge (src_node, dest_node, t, dt, latency, distance);
> + add_edge_to_ddg (g, e);
> }
>
> /* The same as the above function, but it doesn't require a link parameter.
> */
> @@ -237,8 +237,8 @@ create_ddg_dep_no_link (ddg_ptr g, ddg_node_ptr from,
> ddg_node_ptr to,
> enum reg_note dep_kind;
> struct _dep _dep, *dep = &_dep;
>
> - gcc_assert (!DEBUG_INSN_P (to->insn) || d_t == ANTI_DEP);
> - gcc_assert (!DEBUG_INSN_P (from->insn) || d_t == ANTI_DEP);
> + gcc_assert (NONDEBUG_INSN_P (to->insn) || d_t == ANTI_DEP);
> + gcc_assert (NONDEBUG_INSN_P (from->insn) || DEBUG_INSN_P (to->insn));
>
> if (d_t == ANTI_DEP)
> dep_kind = REG_DEP_ANTI;
> @@ -455,8 +455,12 @@ add_inter_loop_mem_dep (ddg_ptr g, ddg_node_ptr from,
> ddg_node_ptr to)
> return;
> else if (from->cuid != to->cuid)
> {
> - create_ddg_dep_no_link (g, from, to, ANTI_DEP, MEM_DEP, 1);
> - if (DEBUG_INSN_P (from->insn) || DEBUG_INSN_P (to->insn))
> + gcc_assert (NONDEBUG_INSN_P (to->insn));
> +
> + if (NONDEBUG_INSN_P (from->insn))
> + create_ddg_dep_no_link (g, from, to, ANTI_DEP, MEM_DEP, 1);
> +
> + if (DEBUG_INSN_P (from->insn))
> create_ddg_dep_no_link (g, to, from, ANTI_DEP, MEM_DEP, 1);
> else
> create_ddg_dep_no_link (g, to, from, TRUE_DEP, MEM_DEP, 1);
> @@ -607,28 +611,34 @@ create_ddg (basic_block bb, int closing_branch_deps)
> if (! INSN_P (insn))
> {
> if (! first_note && NOTE_P (insn)
> - && NOTE_KIND (insn) != NOTE_INSN_BASIC_BLOCK)
> + && NOTE_KIND (insn) != NOTE_INSN_BASIC_BLOCK)
> first_note = insn;
> continue;
> }
> +
> if (JUMP_P (insn))
> {
> gcc_assert (!g->closing_branch);
> g->closing_branch = &g->nodes[i];
> }
> - else if (GET_CODE (PATTERN (insn)) == USE)
> - {
> - if (! first_note)
> - first_note = insn;
> - continue;
> - }
> +
> + if (! first_note)
> + first_note = insn;
> +
> + if (GET_CODE (PATTERN (insn)) == USE)
> + continue;
>
> g->nodes[i].cuid = i;
> g->nodes[i].successors = sbitmap_alloc (num_nodes);
> bitmap_clear (g->nodes[i].successors);
> g->nodes[i].predecessors = sbitmap_alloc (num_nodes);
> bitmap_clear (g->nodes[i].predecessors);
> - g->nodes[i].first_note = (first_note ? first_note : insn);
> +
> + if (! DEBUG_INSN_P (insn))
> + {
> + g->nodes[i].first_note = first_note;
> + first_note = NULL;
> + }
>
> g->nodes[i].aux.count = -1;
> g->nodes[i].max_dist = XCNEWVEC (int, num_nodes);
> @@ -636,7 +646,6 @@ create_ddg (basic_block bb, int closing_branch_deps)
> g->nodes[i].max_dist[j] = -1;
>
> g->nodes[i++].insn = insn;
> - first_note = NULL;
> }
>
> /* We must have found a branch in DDG. */
> @@ -1002,13 +1011,19 @@ order_sccs (ddg_all_sccs_ptr g)
> static void
> check_sccs (ddg_all_sccs_ptr sccs, int num_nodes)
> {
> - int i = 0;
> + int i;
> + unsigned int j;
> auto_sbitmap tmp (num_nodes);
>
> bitmap_clear (tmp);
> for (i = 0; i < sccs->num_sccs; i++)
> {
> + sbitmap_iterator sbi;
> gcc_assert (!bitmap_empty_p (sccs->sccs[i]->nodes));
> + /* Debug insns should not be in found SCCs. */
> + EXECUTE_IF_SET_IN_BITMAP (sccs->sccs[i]->nodes, 0, j, sbi)
> + gcc_assert (NONDEBUG_INSN_P (sccs->ddg->nodes[j].insn));
> +
> /* Verify that every node in sccs is in exactly one strongly
> connected component. */
> gcc_assert (!bitmap_intersect_p (tmp, sccs->sccs[i]->nodes));
> diff --git a/gcc/modulo-sched.c b/gcc/modulo-sched.c
> index e16e023243..3f6aa8c7c2 100644
> --- a/gcc/modulo-sched.c
> +++ b/gcc/modulo-sched.c
> @@ -318,6 +318,7 @@ static rtx_insn *
> ps_first_note (partial_schedule_ptr ps, int id)
> {
> gcc_assert (id < ps->g->num_nodes);
> + gcc_assert (ps->g->nodes[id].first_note);
> return ps->g->nodes[id].first_note;
> }
>
> diff --git a/gcc/testsuite/gcc.dg/pr42631-sms1.c
> b/gcc/testsuite/gcc.dg/pr42631-sms1.c
> new file mode 100644
> index 0000000000..0117276c73
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr42631-sms1.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-options "-g -O1 -funroll-loops -fmodulo-sched -fcompare-debug" } */
> +/* { dg-xfail-if "" { powerpc-ibm-aix* } } */
> +
> +void foo()
> +{
> + unsigned k;
> + while (--k > 0);
> +}
> diff --git a/gcc/testsuite/gcc.dg/pr42631-sms2.c
> b/gcc/testsuite/gcc.dg/pr42631-sms2.c
> new file mode 100644
> index 0000000000..2555da4522
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr42631-sms2.c
> @@ -0,0 +1,9 @@
> +/* { dg-do compile } */
> +/* { dg-options "-g -O1 -funroll-loops -fmodulo-sched
> -fmodulo-sched-allow-regmoves -fcompare-debug" } */
> +/* { dg-xfail-if "" { powerpc-ibm-aix* } } */
> +
> +void foo()
> +{
> + unsigned k;
> + while (--k > 0);
> +}
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
