Fix PR54650

Submitted by Dehao Chen on Sept. 22, 2012, 2:08 a.m.

Details

Message ID CAO2gOZXeHsy1hTaF88gsn=SuVnrQ2aV6XfEkRigmU2YGH9frcw@mail.gmail.com
State New
Headers show

Commit Message

Dehao Chen Sept. 22, 2012, 2:08 a.m.
Hi,

The problem is due to a bug when move_block_to_fn. edge->goto_block
should be updated even when its locus is unknown. This patch also
fixes the way to reset block for expr.

Bootstrapped and pass all gcc regression tests.

Is it okay for trunk?

Thanks,
Dehao

gcc/ChangeLog:
        tree-cfg.c (move_stmt_op): Reset the expr block only
        when necessary.
        (move_block_to_fn): Reset the edge's goto block even
        when the goto locus is unknown.

Comments

Richard Guenther Sept. 24, 2012, 11:10 a.m.
On Sat, Sep 22, 2012 at 4:08 AM, Dehao Chen <dehao@google.com> wrote:
> Hi,
>
> The problem is due to a bug when move_block_to_fn. edge->goto_block
> should be updated even when its locus is unknown. This patch also
> fixes the way to reset block for expr.

I think the machinery is much more fragile (which is probably why
when in SSA form the whole thing is invoked with a NULL_TREE BLOCK).
From looking at how the block tree is mangled:

  /* Rewire BLOCK_SUBBLOCKS of orig_block.  */
  if (orig_block)
    {
      tree block;
      gcc_assert (BLOCK_SUBBLOCKS (DECL_INITIAL (dest_cfun->decl))
                  == NULL_TREE);
      BLOCK_SUBBLOCKS (DECL_INITIAL (dest_cfun->decl))
        = BLOCK_SUBBLOCKS (orig_block);
      for (block = BLOCK_SUBBLOCKS (orig_block);
           block; block = BLOCK_CHAIN (block))
        BLOCK_SUPERCONTEXT (block) = DECL_INITIAL (dest_cfun->decl);
      BLOCK_SUBBLOCKS (orig_block) = NULL_TREE;
    }

I think we want a simpler version of your fix:

> Bootstrapped and pass all gcc regression tests.
>
> Is it okay for trunk?
>
> Thanks,
> Dehao
>
> gcc/ChangeLog:
>         tree-cfg.c (move_stmt_op): Reset the expr block only
>         when necessary.
>         (move_block_to_fn): Reset the edge's goto block even
>         when the goto locus is unknown.
>
> Index: tree-cfg.c
> ===================================================================
> --- tree-cfg.c  (revision 191614)
> +++ tree-cfg.c  (working copy)
> @@ -6013,7 +6013,9 @@ move_stmt_op (tree *tp, int *walk_subtrees, void *
>
>    if (EXPR_P (t))
>      {
> -      if (TREE_BLOCK (t))
> +      if (p->orig_block == NULL_TREE
> +         || TREE_BLOCK (t) == p->orig_block
> +         || TREE_BLOCK (t) == NULL_TREE)
>         TREE_SET_BLOCK (t, p->new_block);

Only replace TREE_BLOCK of t if it was equal to p->orig_block or if
p->orig_block is NULL_TREE _and_ TREE_BLOCK of t is not NULL_TREE.

>      }
>    else if (DECL_P (t) || TREE_CODE (t) == SSA_NAME)
> @@ -6315,7 +6317,7 @@ move_block_to_fn (struct function *dest_cfun, basi
>      }
>
>    FOR_EACH_EDGE (e, ei, bb->succs)
> -    if (!IS_UNKNOWN_LOCATION (e->goto_locus))
> +    if (e->goto_locus)

Please compare against UNKNOWN_LOCATION (which is zero, right)?

Ok with that changes.

Thanks,
Richard.

>        {
>         tree block = LOCATION_BLOCK (e->goto_locus);
>         if (d->orig_block == NULL_TREE

Patch hide | download patch | download mbox

Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 191614)
+++ tree-cfg.c	(working copy)
@@ -6013,7 +6013,9 @@  move_stmt_op (tree *tp, int *walk_subtrees, void *

   if (EXPR_P (t))
     {
-      if (TREE_BLOCK (t))
+      if (p->orig_block == NULL_TREE
+	  || TREE_BLOCK (t) == p->orig_block
+	  || TREE_BLOCK (t) == NULL_TREE)
 	TREE_SET_BLOCK (t, p->new_block);
     }
   else if (DECL_P (t) || TREE_CODE (t) == SSA_NAME)
@@ -6315,7 +6317,7 @@  move_block_to_fn (struct function *dest_cfun, basi
     }

   FOR_EACH_EDGE (e, ei, bb->succs)
-    if (!IS_UNKNOWN_LOCATION (e->goto_locus))
+    if (e->goto_locus)
       {
 	tree block = LOCATION_BLOCK (e->goto_locus);
 	if (d->orig_block == NULL_TREE