Patchwork [rfa] Fix PR44699: bootstrap problem

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

Comments

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

On Wed, 30 Jun 2010, Richard Guenther wrote:

> > -      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.

While it is pre-existing confusion in this function you're right.  See 
below for the fix (just replacing move_ssa_defining_stmt_for_defs by 
SSA_NAME_DEF_STMT() assigns is enough).

> 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.

For sure.  Can I make it nice later and fix the bootstrap problem on 
Darwin first?  The freeze starts soon.

(FWIW the improvement you sent is not equivalent, it will attach a VDEF to 
the last insn when non is needed, and there's some confusion with the 
last==laststore test).


Ciao,
Michael.
--
Richard Guenther - June 30, 2010, 1:55 p.m.
On Wed, Jun 30, 2010 at 3:53 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Wed, 30 Jun 2010, Richard Guenther wrote:
>
>> > -      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.
>
> While it is pre-existing confusion in this function you're right.  See
> below for the fix (just replacing move_ssa_defining_stmt_for_defs by
> SSA_NAME_DEF_STMT() assigns is enough).
>
>> 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.
>
> For sure.  Can I make it nice later and fix the bootstrap problem on
> Darwin first?  The freeze starts soon.

Yes, that works for me.

Richard.

> (FWIW the improvement you sent is not equivalent, it will attach a VDEF to
> the last insn when non is needed, and there's some confusion with the
> last==laststore test).
>
>
> Ciao,
> Michael.
> --
> 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);
> +         SSA_NAME_DEF_STMT (gimple_vdef (stmt)) = laststore;
> +         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));
> +         SSA_NAME_DEF_STMT (gimple_vdef (stmt)) = new_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);
+         SSA_NAME_DEF_STMT (gimple_vdef (stmt)) = laststore;
+         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));
+         SSA_NAME_DEF_STMT (gimple_vdef (stmt)) = new_stmt;
+       }
     }

   gimple_set_location (new_stmt, gimple_location (stmt));