Patchwork [rfa] Fix PR44699: bootstrap problem

login
register
mail settings
Submitter Michael Matz
Date June 30, 2010, 1:25 p.m.
Message ID <Pine.LNX.4.64.1006301522080.18619@wotan.suse.de>
Download mbox | patch
Permalink /patch/57411/
State New
Headers show

Comments

Michael Matz - June 30, 2010, 1:25 p.m.
Hi,

On Tue, 29 Jun 2010, Richard Guenther wrote:

> On Tue, Jun 29, 2010 at 5:45 PM, Michael Matz <matz@suse.de> wrote:
> > Hello,
> >
> > Allocating an array of length num_ssa_names, then doing foldings or other
> > transformations that can create new SSA names, and then iterating that
> > array again as if it had num_ssa_names elements is not a good idea.
> >
> > This fixes the testcase.  I'm going to regstrap this, once the
> > function_arg_advance brokenness is fixed.  Okay, assuming this will work?
> 
> Ok.

While it fixes the testcase (and regstraps on linux) it still breaks on 
Darwin.  We also need to be careful to not attach VDEFs to the generated 
new 'lhs = tmp' assignment if LHS is a gimple reg.  Those VDEFs would be 
removed fairly quickly breaking the vdef/vuse chains again (that was 
pre-existing breakage).

So I'd like to add this hunk to the patch (whole patch regstrapping again 
on x86_64-linux).  Okay for trunk?


Ciao,
Michael.
Diego Novillo - June 30, 2010, 1:28 p.m.
On Wed, Jun 30, 2010 at 09:25, Michael Matz <matz@suse.de> wrote:

>        * gimple-fold.c (gimplify_and_update_call_from_tree): If LHS is
>        a gimple reg, attach the original VDEF to the last store in the
>        sequence.

OK if it passes testing.


Diego.
Richard Guenther - June 30, 2010, 1:29 p.m.
On Wed, Jun 30, 2010 at 3:25 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Tue, 29 Jun 2010, Richard Guenther wrote:
>
>> On Tue, Jun 29, 2010 at 5:45 PM, Michael Matz <matz@suse.de> wrote:
>> > Hello,
>> >
>> > Allocating an array of length num_ssa_names, then doing foldings or other
>> > transformations that can create new SSA names, and then iterating that
>> > array again as if it had num_ssa_names elements is not a good idea.
>> >
>> > This fixes the testcase.  I'm going to regstrap this, once the
>> > function_arg_advance brokenness is fixed.  Okay, assuming this will work?
>>
>> Ok.
>
> While it fixes the testcase (and regstraps on linux) it still breaks on
> Darwin.  We also need to be careful to not attach VDEFs to the generated
> new 'lhs = tmp' assignment if LHS is a gimple reg.  Those VDEFs would be
> removed fairly quickly breaking the vdef/vuse chains again (that was
> pre-existing breakage).
>
> So I'd like to add this hunk to the patch (whole patch regstrapping again
> on x86_64-linux).  Okay for trunk?
>
>
> Ciao,
> Michael.
> --
>        * gimple-fold.c (gimplify_and_update_call_from_tree): If LHS is
>        a gimple reg, attach the original VDEF to the last store in the
>        sequence.
>
> Index: gimple-fold.c
> ===================================================================
> --- gimple-fold.c       (revision 161602)
> +++ gimple-fold.c       (working copy)
> @@ -1150,7 +1150,14 @@ gimplify_and_update_call_from_tree (gimp
>          gsi_insert_before (si_p, last, GSI_NEW_STMT);
>          gsi_next (si_p);
>        }
> -      if (laststore)
> +      if (laststore && is_gimple_reg (lhs))
> +       {
> +         gimple_set_vdef (laststore, gimple_vdef (stmt));
> +         update_stmt (laststore);
> +         move_ssa_defining_stmt_for_defs (laststore, stmt);
> +         laststore = NULL;
> +       }

I don't think using move_ssa_defining_stmt_for_defs is appropriate
in this function.  It will move both virtual and real operand defs.
Instead the real defs (well, that for the lhs only) should be moved
to the last stmt in the sequence and the virtual def to the last store.

The fn also now looks ugly.

RIchard.

> +      else if (laststore)
>        {
>          reaching_vuse = make_ssa_name (gimple_vop (cfun), laststore);
>          gimple_set_vdef (laststore, reaching_vuse);
> @@ -1158,9 +1165,13 @@ gimplify_and_update_call_from_tree (gimp
>          laststore = NULL;
>        }
>       new_stmt = gimple_build_assign (lhs, tmp);
> -      gimple_set_vuse (new_stmt, reaching_vuse);
> -      gimple_set_vdef (new_stmt, gimple_vdef (stmt));
> -      move_ssa_defining_stmt_for_defs (new_stmt, stmt);
> +      if (!is_gimple_reg (tmp))
> +       gimple_set_vuse (new_stmt, reaching_vuse);
> +      if (!is_gimple_reg (lhs))
> +       {
> +         gimple_set_vdef (new_stmt, gimple_vdef (stmt));
> +         move_ssa_defining_stmt_for_defs (new_stmt, stmt);
> +       }
>     }
>
>   gimple_set_location (new_stmt, gimple_location (stmt));

Patch

Index: gimple-fold.c
===================================================================
--- gimple-fold.c	(revision 161602)
+++ gimple-fold.c	(working copy)
@@ -1150,7 +1150,14 @@  gimplify_and_update_call_from_tree (gimp
 	  gsi_insert_before (si_p, last, GSI_NEW_STMT);
 	  gsi_next (si_p);
 	}
-      if (laststore)
+      if (laststore && is_gimple_reg (lhs))
+	{
+	  gimple_set_vdef (laststore, gimple_vdef (stmt));
+	  update_stmt (laststore);
+	  move_ssa_defining_stmt_for_defs (laststore, stmt);
+	  laststore = NULL;
+	}
+      else if (laststore)
 	{
 	  reaching_vuse = make_ssa_name (gimple_vop (cfun), laststore);
 	  gimple_set_vdef (laststore, reaching_vuse);
@@ -1158,9 +1165,13 @@  gimplify_and_update_call_from_tree (gimp
 	  laststore = NULL;
 	}
       new_stmt = gimple_build_assign (lhs, tmp);
-      gimple_set_vuse (new_stmt, reaching_vuse);
-      gimple_set_vdef (new_stmt, gimple_vdef (stmt));
-      move_ssa_defining_stmt_for_defs (new_stmt, stmt);
+      if (!is_gimple_reg (tmp))
+	gimple_set_vuse (new_stmt, reaching_vuse);
+      if (!is_gimple_reg (lhs))
+	{
+	  gimple_set_vdef (new_stmt, gimple_vdef (stmt));
+	  move_ssa_defining_stmt_for_defs (new_stmt, stmt);
+	}
     }
 
   gimple_set_location (new_stmt, gimple_location (stmt));