[PATCH] Fix other fallout of the -g -O0 edge locus/block patch

[PATCH] Fix other fallout of the -g -O0 edge locus/block patch

[Jakub Jelinek]

Hi!

On the 4.3-RH backport of the patch I had a report of an ICE in change_scope
on a large testcase.  Turns out remove_unused_locals removed a BLOCK as
unused when it was only referenced from edge's goto_block.  Fixed by the
first hunk.  The tree-cfg.c changes are just to allow GC etc. of BLOCKs
from edges that have goto_locus == 0, as we don't care about their blocks
anyway.  And the lowering changes are to make sure we don't create
GIMPLE stmts that have locus set, but still have gimple_block NULL, which
happens when new stmts are created during lowering.  While only the case
where GIMPLE_GOTO with non-0 gimple_location, but NULL gimple_block is
problematic for tree-cfg.c and onwards, it is IMHO desirable to use correct
block in all cases.

Bootstrap in progress on x86_64-linux, ok for trunk if it + regtest passes?

2008-10-09  Jakub Jelinek  <jakub@redhat.com>

	* tree-ssa-live.c (remove_unused_locals): Mark all edge's goto_block
	as used.
	* gimple-low.c (lower_function_body, lower_gimple_return,
	lower_builtin_setjmp): Set gimple_block on the newly created stmts.
	* tree-cfg.c (make_cond_expr_edges, make_goto_expr_edges): Only set
	goto_block on edges if goto_locus is known.

--- gcc/tree-ssa-live.c.jj	2008-10-04 11:24:32.000000000 +0200
+++ gcc/tree-ssa-live.c	2008-10-09 00:36:47.000000000 +0200
@@ -600,6 +600,8 @@ remove_unused_locals (void)
     {
       gimple_stmt_iterator gsi;
       size_t i;
+      edge_iterator ei;
+      edge e;
 
       /* Walk the statements.  */
       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
@@ -634,6 +636,10 @@ remove_unused_locals (void)
 	      mark_all_vars_used (&arg, NULL);
             }
         }
+
+      FOR_EACH_EDGE (e, ei, bb->succs)
+	if (e->goto_locus)
+	  TREE_USED (e->goto_block) = true;
     }
 
   /* Remove unmarked local vars from local_decls.  */
--- gcc/gimple-low.c.jj	2008-09-05 12:56:32.000000000 +0200
+++ gcc/gimple-low.c	2008-10-09 00:33:22.000000000 +0200
@@ -133,6 +133,7 @@ lower_function_body (void)
     {
       x = gimple_build_return (NULL);
       gimple_set_location (x, cfun->function_end_locus);
+      gimple_set_block (x, DECL_INITIAL (current_function_decl));
       gsi_insert_after (&i, x, GSI_CONTINUE_LINKING);
     }
 
@@ -659,6 +660,7 @@ lower_gimple_return (gimple_stmt_iterato
  found:
   t = gimple_build_goto (tmp_rs.label);
   gimple_set_location (t, gimple_location (stmt));
+  gimple_set_block (t, gimple_block (stmt));
   gsi_insert_before (gsi, t, GSI_SAME_STMT);
   gsi_remove (gsi, false);
 }
@@ -736,6 +738,7 @@ lower_builtin_setjmp (gimple_stmt_iterat
   t = implicit_built_in_decls[BUILT_IN_SETJMP_SETUP];
   g = gimple_build_call (t, 2, gimple_call_arg (stmt, 0), arg);
   gimple_set_location (g, gimple_location (stmt));
+  gimple_set_block (g, gimple_block (stmt));
   gsi_insert_before (gsi, g, GSI_SAME_STMT);
 
   /* Build 'DEST = 0' and insert.  */
@@ -744,6 +747,7 @@ lower_builtin_setjmp (gimple_stmt_iterat
       g = gimple_build_assign (dest, fold_convert (TREE_TYPE (dest),
 						   integer_zero_node));
       gimple_set_location (g, gimple_location (stmt));
+      gimple_set_block (g, gimple_block (stmt));
       gsi_insert_before (gsi, g, GSI_SAME_STMT);
     }
 
@@ -760,6 +764,7 @@ lower_builtin_setjmp (gimple_stmt_iterat
   t = implicit_built_in_decls[BUILT_IN_SETJMP_RECEIVER];
   g = gimple_build_call (t, 1, arg);
   gimple_set_location (g, gimple_location (stmt));
+  gimple_set_block (g, gimple_block (stmt));
   gsi_insert_before (gsi, g, GSI_SAME_STMT);
 
   /* Build 'DEST = 1' and insert.  */
@@ -768,6 +773,7 @@ lower_builtin_setjmp (gimple_stmt_iterat
       g = gimple_build_assign (dest, fold_convert (TREE_TYPE (dest),
 						   integer_one_node));
       gimple_set_location (g, gimple_location (stmt));
+      gimple_set_block (g, gimple_block (stmt));
       gsi_insert_before (gsi, g, GSI_SAME_STMT);
     }
 
--- gcc/tree-cfg.c.jj	2008-10-08 11:50:40.000000000 +0200
+++ gcc/tree-cfg.c	2008-10-09 00:34:38.000000000 +0200
@@ -658,12 +658,14 @@ make_cond_expr_edges (basic_block bb)
 
   e = make_edge (bb, then_bb, EDGE_TRUE_VALUE);
   e->goto_locus = gimple_location (then_stmt);
-  e->goto_block = gimple_block (then_stmt);
+  if (e->goto_locus)
+    e->goto_block = gimple_block (then_stmt);
   e = make_edge (bb, else_bb, EDGE_FALSE_VALUE);
   if (e)
     {
       e->goto_locus = gimple_location (else_stmt);
-      e->goto_block = gimple_block (else_stmt);
+      if (e->goto_locus)
+	e->goto_block = gimple_block (else_stmt);
     }
 
   /* We do not need the labels anymore.  */
@@ -853,7 +855,8 @@ make_goto_expr_edges (basic_block bb)
       tree dest = gimple_goto_dest (goto_t);
       edge e = make_edge (bb, label_to_block (dest), EDGE_FALLTHRU);
       e->goto_locus = gimple_location (goto_t);
-      e->goto_block = gimple_block (goto_t);
+      if (e->goto_locus)
+	e->goto_block = gimple_block (goto_t);
       gsi_remove (&last, true);
       return;
     }

	Jakub

Re: [PATCH] Fix other fallout of the -g -O0 edge locus/block patch

[Richard Guenther]

On Thu, Oct 9, 2008 at 12:59 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> On the 4.3-RH backport of the patch I had a report of an ICE in change_scope
> on a large testcase.  Turns out remove_unused_locals removed a BLOCK as
> unused when it was only referenced from edge's goto_block.  Fixed by the
> first hunk.  The tree-cfg.c changes are just to allow GC etc. of BLOCKs
> from edges that have goto_locus == 0, as we don't care about their blocks
> anyway.  And the lowering changes are to make sure we don't create
> GIMPLE stmts that have locus set, but still have gimple_block NULL, which
> happens when new stmts are created during lowering.  While only the case
> where GIMPLE_GOTO with non-0 gimple_location, but NULL gimple_block is
> problematic for tree-cfg.c and onwards, it is IMHO desirable to use correct
> block in all cases.

I guess we could verify this in verify_stmts?

> Bootstrap in progress on x86_64-linux, ok for trunk if it + regtest passes?

Ok.

Thanks,
Richard.

> 2008-10-09  Jakub Jelinek  <jakub@redhat.com>
>
>        * tree-ssa-live.c (remove_unused_locals): Mark all edge's goto_block
>        as used.
>        * gimple-low.c (lower_function_body, lower_gimple_return,
>        lower_builtin_setjmp): Set gimple_block on the newly created stmts.
>        * tree-cfg.c (make_cond_expr_edges, make_goto_expr_edges): Only set
>        goto_block on edges if goto_locus is known.
>
> --- gcc/tree-ssa-live.c.jj      2008-10-04 11:24:32.000000000 +0200
> +++ gcc/tree-ssa-live.c 2008-10-09 00:36:47.000000000 +0200
> @@ -600,6 +600,8 @@ remove_unused_locals (void)
>     {
>       gimple_stmt_iterator gsi;
>       size_t i;
> +      edge_iterator ei;
> +      edge e;
>
>       /* Walk the statements.  */
>       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> @@ -634,6 +636,10 @@ remove_unused_locals (void)
>              mark_all_vars_used (&arg, NULL);
>             }
>         }
> +
> +      FOR_EACH_EDGE (e, ei, bb->succs)
> +       if (e->goto_locus)
> +         TREE_USED (e->goto_block) = true;
>     }
>
>   /* Remove unmarked local vars from local_decls.  */
> --- gcc/gimple-low.c.jj 2008-09-05 12:56:32.000000000 +0200
> +++ gcc/gimple-low.c    2008-10-09 00:33:22.000000000 +0200
> @@ -133,6 +133,7 @@ lower_function_body (void)
>     {
>       x = gimple_build_return (NULL);
>       gimple_set_location (x, cfun->function_end_locus);
> +      gimple_set_block (x, DECL_INITIAL (current_function_decl));
>       gsi_insert_after (&i, x, GSI_CONTINUE_LINKING);
>     }
>
> @@ -659,6 +660,7 @@ lower_gimple_return (gimple_stmt_iterato
>  found:
>   t = gimple_build_goto (tmp_rs.label);
>   gimple_set_location (t, gimple_location (stmt));
> +  gimple_set_block (t, gimple_block (stmt));
>   gsi_insert_before (gsi, t, GSI_SAME_STMT);
>   gsi_remove (gsi, false);
>  }
> @@ -736,6 +738,7 @@ lower_builtin_setjmp (gimple_stmt_iterat
>   t = implicit_built_in_decls[BUILT_IN_SETJMP_SETUP];
>   g = gimple_build_call (t, 2, gimple_call_arg (stmt, 0), arg);
>   gimple_set_location (g, gimple_location (stmt));
> +  gimple_set_block (g, gimple_block (stmt));
>   gsi_insert_before (gsi, g, GSI_SAME_STMT);
>
>   /* Build 'DEST = 0' and insert.  */
> @@ -744,6 +747,7 @@ lower_builtin_setjmp (gimple_stmt_iterat
>       g = gimple_build_assign (dest, fold_convert (TREE_TYPE (dest),
>                                                   integer_zero_node));
>       gimple_set_location (g, gimple_location (stmt));
> +      gimple_set_block (g, gimple_block (stmt));
>       gsi_insert_before (gsi, g, GSI_SAME_STMT);
>     }
>
> @@ -760,6 +764,7 @@ lower_builtin_setjmp (gimple_stmt_iterat
>   t = implicit_built_in_decls[BUILT_IN_SETJMP_RECEIVER];
>   g = gimple_build_call (t, 1, arg);
>   gimple_set_location (g, gimple_location (stmt));
> +  gimple_set_block (g, gimple_block (stmt));
>   gsi_insert_before (gsi, g, GSI_SAME_STMT);
>
>   /* Build 'DEST = 1' and insert.  */
> @@ -768,6 +773,7 @@ lower_builtin_setjmp (gimple_stmt_iterat
>       g = gimple_build_assign (dest, fold_convert (TREE_TYPE (dest),
>                                                   integer_one_node));
>       gimple_set_location (g, gimple_location (stmt));
> +      gimple_set_block (g, gimple_block (stmt));
>       gsi_insert_before (gsi, g, GSI_SAME_STMT);
>     }
>
> --- gcc/tree-cfg.c.jj   2008-10-08 11:50:40.000000000 +0200
> +++ gcc/tree-cfg.c      2008-10-09 00:34:38.000000000 +0200
> @@ -658,12 +658,14 @@ make_cond_expr_edges (basic_block bb)
>
>   e = make_edge (bb, then_bb, EDGE_TRUE_VALUE);
>   e->goto_locus = gimple_location (then_stmt);
> -  e->goto_block = gimple_block (then_stmt);
> +  if (e->goto_locus)
> +    e->goto_block = gimple_block (then_stmt);
>   e = make_edge (bb, else_bb, EDGE_FALSE_VALUE);
>   if (e)
>     {
>       e->goto_locus = gimple_location (else_stmt);
> -      e->goto_block = gimple_block (else_stmt);
> +      if (e->goto_locus)
> +       e->goto_block = gimple_block (else_stmt);
>     }
>
>   /* We do not need the labels anymore.  */
> @@ -853,7 +855,8 @@ make_goto_expr_edges (basic_block bb)
>       tree dest = gimple_goto_dest (goto_t);
>       edge e = make_edge (bb, label_to_block (dest), EDGE_FALLTHRU);
>       e->goto_locus = gimple_location (goto_t);
> -      e->goto_block = gimple_block (goto_t);
> +      if (e->goto_locus)
> +       e->goto_block = gimple_block (goto_t);
>       gsi_remove (&last, true);
>       return;
>     }
>
>        Jakub
>

Re: [PATCH] Fix other fallout of the -g -O0 edge locus/block patch

[Cary Coutant]

> On the 4.3-RH backport of the patch I had a report of an ICE in change_scope
> on a large testcase.  Turns out remove_unused_locals removed a BLOCK as
> unused when it was only referenced from edge's goto_block.  Fixed by the
> first hunk.  The tree-cfg.c changes are just to allow GC etc. of BLOCKs
> from edges that have goto_locus == 0, as we don't care about their blocks
> anyway.  And the lowering changes are to make sure we don't create
> GIMPLE stmts that have locus set, but still have gimple_block NULL, which
> happens when new stmts are created during lowering.  While only the case
> where GIMPLE_GOTO with non-0 gimple_location, but NULL gimple_block is
> problematic for tree-cfg.c and onwards, it is IMHO desirable to use correct
> block in all cases.

>        * tree-cfg.c (make_cond_expr_edges, make_goto_expr_edges): Only set
>        goto_block on edges if goto_locus is known.

I've been trying to understand the logic in make_cond_expr_edges
(setting e->goto_block only when e->goto_locus != 0). In
make_cond_expr_edges, when will goto_locus ever be non-zero? It seems
that the first statements of both the then and the else blocks are
always labels, and the label statements always have a gimple_location
of 0. To verify, I tried adding "gcc_assert (e->goto_locus == 0)" for
both the then and else branches, and ran the entire test suite without
that assert ever firing. I'm sure I'm missing something, but what?

-cary