Re: [PATCH] gimplify: Fix -fcompare-debug differences caused by gimplify_body [PR94281]
On Thu, 26 Mar 2020, Jakub Jelinek wrote:
> Hi!
>
> The following testcase FAILs, because gimplify_body adds a GIMPLE_NOP only
> when there are no statements in the function and with -g there is a
> DEBUG_BEGIN_STMT, so it doesn't add it and due to -fno-tree-dce that never
> gets removed afterwards. Similarly, if the body seq after gimplification
> contains some DEBUG_BEGIN_STMTs plus a single gbind, then we could behave
> differently between -g0 and -g, by using that gbind as the body in the -g0
> case and not in the -g case.
> This patch fixes that by ignoring DEBUG_BEGIN_STMTs (other debug stmts can't
> appear at this point yet thankfully) during decisions and if we pick the
> single gbind and there are DEBUG_BEGIN_STMTs next to it, it moves them into
> the gbind.
> While debugging this, I found also a bug in the gimple_seq_last_nondebug_stmt
> function, for a seq that has a single non-DEBUG_BEGIN_STMT statement
> followed by one or more DEBUG_BEGIN_STMTs it would return NULL rather than
> the first statement.
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?
OK.
Thanks,
Richard.
> 2020-03-26 Jakub Jelinek
>
> PR debug/94281
> * gimple.h (gimple_seq_first_nondebug_stmt): New function.
> (gimple_seq_last_nondebug_stmt): Don't return NULL if seq contains
> a single non-debug stmt followed by one or more debug stmts.
> * gimplify.c (gimplify_body): Use gimple_seq_first_nondebug_stmt
> instead of gimple_seq_first_stmt, use gimple_seq_first_nondebug_stmt
> and gimple_seq_last_nondebug_stmt instead of gimple_seq_first and
> gimple_seq_last to check if outer_stmt gbind could be reused and
> if yes and it is surrounded by any debug stmts, move them into the
> gbind body.
>
> * g++.dg/debug/pr94281.C: New test.
>
> --- gcc/gimple.h.jj 2020-03-25 14:34:07.696199561 +0100
> +++ gcc/gimple.h 2020-03-25 15:26:38.045052864 +0100
> @@ -4728,6 +4728,18 @@ is_gimple_debug (const gimple *gs)
> }
>
>
> +/* Return the first nondebug statement in GIMPLE sequence S. */
> +
> +static inline gimple *
> +gimple_seq_first_nondebug_stmt (gimple_seq s)
> +{
> + gimple_seq_node n = gimple_seq_first (s);
> + while (n && is_gimple_debug (n))
> +n = n->next;
> + return n;
> +}
> +
> +
> /* Return the last nondebug statement in GIMPLE sequence S. */
>
> static inline gimple *
> @@ -4737,7 +4749,7 @@ gimple_seq_last_nondebug_stmt (gimple_se
>for (n = gimple_seq_last (s);
> n && is_gimple_debug (n);
> n = n->prev)
> -if (n->prev == s)
> +if (n == s)
>return NULL;
>return n;
> }
> --- gcc/gimplify.c.jj 2020-03-25 14:34:07.722199170 +0100
> +++ gcc/gimplify.c2020-03-25 14:41:46.447348434 +0100
> @@ -14849,7 +14849,7 @@ gimplify_body (tree fndecl, bool do_parm
>/* Gimplify the function's body. */
>seq = NULL;
>gimplify_stmt (_SAVED_TREE (fndecl), );
> - outer_stmt = gimple_seq_first_stmt (seq);
> + outer_stmt = gimple_seq_first_nondebug_stmt (seq);
>if (!outer_stmt)
> {
>outer_stmt = gimple_build_nop ();
> @@ -14859,8 +14859,37 @@ gimplify_body (tree fndecl, bool do_parm
>/* The body must contain exactly one statement, a GIMPLE_BIND. If this is
> not the case, wrap everything in a GIMPLE_BIND to make it so. */
>if (gimple_code (outer_stmt) == GIMPLE_BIND
> - && gimple_seq_first (seq) == gimple_seq_last (seq))
> -outer_bind = as_a (outer_stmt);
> + && (gimple_seq_first_nondebug_stmt (seq)
> + == gimple_seq_last_nondebug_stmt (seq)))
> +{
> + outer_bind = as_a (outer_stmt);
> + if (gimple_seq_first_stmt (seq) != outer_stmt
> + || gimple_seq_last_stmt (seq) != outer_stmt)
> + {
> + /* If there are debug stmts before or after outer_stmt, move them
> + inside of outer_bind body. */
> + gimple_stmt_iterator gsi = gsi_for_stmt (outer_stmt, );
> + gimple_seq second_seq = NULL;
> + if (gimple_seq_first_stmt (seq) != outer_stmt
> + && gimple_seq_last_stmt (seq) != outer_stmt)
> + {
> + second_seq = gsi_split_seq_after (gsi);
> + gsi_remove (, false);
> + }
> + else if (gimple_seq_first_stmt (seq) != outer_stmt)
> + gsi_remove (, false);
> + else
> + {
> + gsi_remove (, false);
> + second_seq = seq;
> + seq = NULL;
> + }
> + gimple_seq_add_seq_without_update (,
> + gimple_bind_body (outer_bind));
> + gimple_seq_add_seq_without_update (, second_seq);
> + gimple_bind_set_body (outer_bind, seq);
> + }
> +}
>else
> outer_bind = gimple_build_bind (NULL_TREE, seq, NULL);
>
> --- gcc/testsuite/g++.dg/debug/pr94281.C.jj 2020-03-25 14:47:11.241498952
> +0100
> +++ gcc/testsuite/g++.dg/debug/pr94281.C 2020-03-25 14:45:32.637971196
> +0100
> @@ -0,0 +1,11
[PATCH] gimplify: Fix -fcompare-debug differences caused by gimplify_body [PR94281]
Hi!
The following testcase FAILs, because gimplify_body adds a GIMPLE_NOP only
when there are no statements in the function and with -g there is a
DEBUG_BEGIN_STMT, so it doesn't add it and due to -fno-tree-dce that never
gets removed afterwards. Similarly, if the body seq after gimplification
contains some DEBUG_BEGIN_STMTs plus a single gbind, then we could behave
differently between -g0 and -g, by using that gbind as the body in the -g0
case and not in the -g case.
This patch fixes that by ignoring DEBUG_BEGIN_STMTs (other debug stmts can't
appear at this point yet thankfully) during decisions and if we pick the
single gbind and there are DEBUG_BEGIN_STMTs next to it, it moves them into
the gbind.
While debugging this, I found also a bug in the gimple_seq_last_nondebug_stmt
function, for a seq that has a single non-DEBUG_BEGIN_STMT statement
followed by one or more DEBUG_BEGIN_STMTs it would return NULL rather than
the first statement.
Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?
2020-03-26 Jakub Jelinek
PR debug/94281
* gimple.h (gimple_seq_first_nondebug_stmt): New function.
(gimple_seq_last_nondebug_stmt): Don't return NULL if seq contains
a single non-debug stmt followed by one or more debug stmts.
* gimplify.c (gimplify_body): Use gimple_seq_first_nondebug_stmt
instead of gimple_seq_first_stmt, use gimple_seq_first_nondebug_stmt
and gimple_seq_last_nondebug_stmt instead of gimple_seq_first and
gimple_seq_last to check if outer_stmt gbind could be reused and
if yes and it is surrounded by any debug stmts, move them into the
gbind body.
* g++.dg/debug/pr94281.C: New test.
--- gcc/gimple.h.jj 2020-03-25 14:34:07.696199561 +0100
+++ gcc/gimple.h2020-03-25 15:26:38.045052864 +0100
@@ -4728,6 +4728,18 @@ is_gimple_debug (const gimple *gs)
}
+/* Return the first nondebug statement in GIMPLE sequence S. */
+
+static inline gimple *
+gimple_seq_first_nondebug_stmt (gimple_seq s)
+{
+ gimple_seq_node n = gimple_seq_first (s);
+ while (n && is_gimple_debug (n))
+n = n->next;
+ return n;
+}
+
+
/* Return the last nondebug statement in GIMPLE sequence S. */
static inline gimple *
@@ -4737,7 +4749,7 @@ gimple_seq_last_nondebug_stmt (gimple_se
for (n = gimple_seq_last (s);
n && is_gimple_debug (n);
n = n->prev)
-if (n->prev == s)
+if (n == s)
return NULL;
return n;
}
--- gcc/gimplify.c.jj 2020-03-25 14:34:07.722199170 +0100
+++ gcc/gimplify.c 2020-03-25 14:41:46.447348434 +0100
@@ -14849,7 +14849,7 @@ gimplify_body (tree fndecl, bool do_parm
/* Gimplify the function's body. */
seq = NULL;
gimplify_stmt (_SAVED_TREE (fndecl), );
- outer_stmt = gimple_seq_first_stmt (seq);
+ outer_stmt = gimple_seq_first_nondebug_stmt (seq);
if (!outer_stmt)
{
outer_stmt = gimple_build_nop ();
@@ -14859,8 +14859,37 @@ gimplify_body (tree fndecl, bool do_parm
/* The body must contain exactly one statement, a GIMPLE_BIND. If this is
not the case, wrap everything in a GIMPLE_BIND to make it so. */
if (gimple_code (outer_stmt) == GIMPLE_BIND
- && gimple_seq_first (seq) == gimple_seq_last (seq))
-outer_bind = as_a (outer_stmt);
+ && (gimple_seq_first_nondebug_stmt (seq)
+ == gimple_seq_last_nondebug_stmt (seq)))
+{
+ outer_bind = as_a (outer_stmt);
+ if (gimple_seq_first_stmt (seq) != outer_stmt
+ || gimple_seq_last_stmt (seq) != outer_stmt)
+ {
+ /* If there are debug stmts before or after outer_stmt, move them
+inside of outer_bind body. */
+ gimple_stmt_iterator gsi = gsi_for_stmt (outer_stmt, );
+ gimple_seq second_seq = NULL;
+ if (gimple_seq_first_stmt (seq) != outer_stmt
+ && gimple_seq_last_stmt (seq) != outer_stmt)
+ {
+ second_seq = gsi_split_seq_after (gsi);
+ gsi_remove (, false);
+ }
+ else if (gimple_seq_first_stmt (seq) != outer_stmt)
+ gsi_remove (, false);
+ else
+ {
+ gsi_remove (, false);
+ second_seq = seq;
+ seq = NULL;
+ }
+ gimple_seq_add_seq_without_update (,
+gimple_bind_body (outer_bind));
+ gimple_seq_add_seq_without_update (, second_seq);
+ gimple_bind_set_body (outer_bind, seq);
+ }
+}
else
outer_bind = gimple_build_bind (NULL_TREE, seq, NULL);
--- gcc/testsuite/g++.dg/debug/pr94281.C.jj 2020-03-25 14:47:11.241498952
+0100
+++ gcc/testsuite/g++.dg/debug/pr94281.C2020-03-25 14:45:32.637971196
+0100
@@ -0,0 +1,11 @@
+// PR debug/94281
+// { dg-do compile }
+// { dg-options "-O1 -fno-tree-dce -fipa-icf -fno-tree-forwprop
-fcompare-debug" }
+
+void fn1()
+{
+}
+void fn2()
+{
+ ;
+}
Jakub
