Patchwork [PR,45565] Update SSA after stmt redirection

login
register
mail settings
Submitter Richard Guenther
Date Sept. 23, 2010, 10:15 a.m.
Message ID <AANLkTinnhkUx6Y7xqGhBcLeP8Z1sMbVm-VVj86Rqj_AS@mail.gmail.com>
Download mbox | patch
Permalink /patch/65522/
State New
Headers show

Comments

Richard Guenther - Sept. 23, 2010, 10:15 a.m.
On Thu, Sep 23, 2010 at 11:16 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> the patch below fixes PR 45565 by updating SSA after inline transform
> even if we don't inline anything and only redirect an edge.  I'm a bit
> surprised this is necessary since we used to assert !need_ssa_update_p
> in these cases
> (http://gcc.gnu.org/ml/gcc-patches/2010-04/msg01012.html) ...but well,
> in inline transform this does not matter.
>
> Bootstrapped and tested on x86_64-linux.  OK for trunk?

No.

The issue seems to be that we call update_stmt during gsi_replace
with still the old fndecl on the stmt, and that old fndecl now became
pure and thus that update_stmt dropped the VDEF.  But the clone
with the skipped args is _not_ marked pure for some reason and
thus the following update_stmt gets back the VDEF but now requires
renaming it.

So I see a missed optimization here, and the PR can be fixed with


instead.  Testing that now.

Richard.

> Thanks,
>
> Martin
>
>
> 2010-09-22  Martin Jambor  <mjambor@suse.cz>
>
>        PR middle-end/45565
>        * cgraphunit.c (cgraph_redirect_edge_call_stmt_to_callee): Return NULL
>        if nothing is changed.
>        * tree-inline.c (copy_bb): Handle that
>        cgraph_redirect_edge_call_stmt_to_callee can return NULL.
>        * ipa-inline.c (inline_transform): Update SSA if
>        cgraph_redirect_edge_call_stmt_to_callee returns non-NULL.
>
>        * testsuite/g++.dg/ipa/pr45565.C: New test.
>
>
> Index: icln/gcc/cgraphunit.c
> ===================================================================
> --- icln.orig/gcc/cgraphunit.c
> +++ icln/gcc/cgraphunit.c
> @@ -2113,7 +2113,8 @@ cgraph_materialize_clone (struct cgraph_
>  }
>
>  /* If necessary, change the function declaration in the call statement
> -   associated with E so that it corresponds to the edge callee.  */
> +   associated with E so that it corresponds to the edge callee.  Return the new
> +   statement or NULL if nothing was changed.  */
>
>  gimple
>  cgraph_redirect_edge_call_stmt_to_callee (struct cgraph_edge *e)
> @@ -2128,7 +2129,7 @@ cgraph_redirect_edge_call_stmt_to_callee
>       || decl == e->callee->decl
>       /* Don't update call from same body alias to the real function.  */
>       || (decl && cgraph_get_node (decl) == cgraph_get_node (e->callee->decl)))
> -    return e->call_stmt;
> +    return NULL;
>
>  #ifdef ENABLE_CHECKING
>   if (decl)
> Index: icln/gcc/ipa-inline.c
> ===================================================================
> --- icln.orig/gcc/ipa-inline.c
> +++ icln/gcc/ipa-inline.c
> @@ -2161,7 +2161,8 @@ inline_transform (struct cgraph_node *no
>
>   for (e = node->callees; e; e = e->next_callee)
>     {
> -      cgraph_redirect_edge_call_stmt_to_callee (e);
> +      if (cgraph_redirect_edge_call_stmt_to_callee (e))
> +       todo = TODO_update_ssa;
>       if (!e->inline_failed || warn_inline)
>         inline_p = true;
>     }
> @@ -2169,7 +2170,7 @@ inline_transform (struct cgraph_node *no
>   if (inline_p)
>     {
>       timevar_push (TV_INTEGRATION);
> -      todo = optimize_inline_calls (current_function_decl);
> +      todo |= optimize_inline_calls (current_function_decl);
>       timevar_pop (TV_INTEGRATION);
>     }
>   cfun->always_inline_functions_inlined = true;
> Index: icln/gcc/tree-inline.c
> ===================================================================
> --- icln.orig/gcc/tree-inline.c
> +++ icln/gcc/tree-inline.c
> @@ -1679,6 +1679,7 @@ copy_bb (copy_body_data *id, basic_block
>                  if (edge)
>                    {
>                      int edge_freq = edge->frequency;
> +                     gimple n_stmt;
>                      edge = cgraph_clone_edge (edge, id->dst_node, stmt,
>                                                gimple_uid (stmt),
>                                                REG_BR_PROB_BASE, CGRAPH_FREQ_BASE,
> @@ -1704,7 +1705,9 @@ copy_bb (copy_body_data *id, basic_block
>                                   bb->frequency,
>                                   copy_basic_block->frequency);
>                        }
> -                     stmt = cgraph_redirect_edge_call_stmt_to_callee (edge);
> +                     n_stmt = cgraph_redirect_edge_call_stmt_to_callee (edge);
> +                     if (n_stmt)
> +                       stmt = n_stmt;
>                    }
>                  break;
>
> Index: icln/gcc/testsuite/g++.dg/ipa/pr45565.C
> ===================================================================
> --- /dev/null
> +++ icln/gcc/testsuite/g++.dg/ipa/pr45565.C
> @@ -0,0 +1,28 @@
> +// { dg-do compile }
> +// { dg-options "-O -fno-toplevel-reorder -fno-inline -fipa-cp -fipa-cp-clone -fkeep-inline-functions" }
> +
> +template < typename Derived > struct AnyMatrixBase
> +{
> +};
> +
> +struct Matrix Random ();
> +
> +struct Matrix:AnyMatrixBase < Matrix >
> +{
> +  void bar ()
> +  {
> +    throw;
> +  }
> +  void foo (Matrix other)
> +  {
> +    bar ();
> +    Matrix (AnyMatrixBase < Matrix > (Random ()));
> +  }
> +  template
> +    < typename OtherDerived > Matrix (AnyMatrixBase < OtherDerived > other)
> +  {
> +    foo (other);
> +  }
> +};
> +
> +Matrix x (Random ());
>
Jan Hubicka - Sept. 23, 2010, 12:24 p.m.
> On Thu, Sep 23, 2010 at 11:16 AM, Martin Jambor <mjambor@suse.cz> wrote:
> > Hi,
> >
> > the patch below fixes PR 45565 by updating SSA after inline transform
> > even if we don't inline anything and only redirect an edge.  I'm a bit
> > surprised this is necessary since we used to assert !need_ssa_update_p
> > in these cases
> > (http://gcc.gnu.org/ml/gcc-patches/2010-04/msg01012.html) ...but well,
> > in inline transform this does not matter.
> >
> > Bootstrapped and tested on x86_64-linux.  OK for trunk?
> 
> No.
> 
> The issue seems to be that we call update_stmt during gsi_replace
> with still the old fndecl on the stmt, and that old fndecl now became
> pure and thus that update_stmt dropped the VDEF.  But the clone
> with the skipped args is _not_ marked pure for some reason and
> thus the following update_stmt gets back the VDEF but now requires
> renaming it.
Hmm, this sounds like bug in copying decl for new function. 
Definitly should be fixed..
We are very lousy on copying and updating attributes when producing clones
in general.

Honza
> 
> So I see a missed optimization here, and the PR can be fixed with
> 
> Index: cgraphunit.c
> ===================================================================
> --- cgraphunit.c        (revision 164550)
> +++ cgraphunit.c        (working copy)
> @@ -2159,6 +2159,7 @@ cgraph_redirect_edge_call_stmt_to_callee
>        new_stmt
>         = gimple_call_copy_skip_args (e->call_stmt,
>                                       e->callee->clone.combined_args_to_skip);
> +      gimple_call_set_fndecl (new_stmt, e->callee->decl);
> 
>        if (gimple_vdef (new_stmt)
>           && TREE_CODE (gimple_vdef (new_stmt)) == SSA_NAME)
> @@ -2168,10 +2169,11 @@ cgraph_redirect_edge_call_stmt_to_callee
>        gsi_replace (&gsi, new_stmt, true);
>      }
>    else
> -    new_stmt = e->call_stmt;
> -
> -  gimple_call_set_fndecl (new_stmt, e->callee->decl);
> -  update_stmt (new_stmt);
> +    {
> +      new_stmt = e->call_stmt;
> +      gimple_call_set_fndecl (new_stmt, e->callee->decl);
> +      update_stmt (new_stmt);
> +    }
> 
>    cgraph_set_call_stmt_including_clones (e->caller, e->call_stmt, new_stmt);
> 
> 
> instead.  Testing that now.
> 
> Richard.
> 
> > Thanks,
> >
> > Martin
> >
> >
> > 2010-09-22  Martin Jambor  <mjambor@suse.cz>
> >
> >        PR middle-end/45565
> >        * cgraphunit.c (cgraph_redirect_edge_call_stmt_to_callee): Return NULL
> >        if nothing is changed.
> >        * tree-inline.c (copy_bb): Handle that
> >        cgraph_redirect_edge_call_stmt_to_callee can return NULL.
> >        * ipa-inline.c (inline_transform): Update SSA if
> >        cgraph_redirect_edge_call_stmt_to_callee returns non-NULL.
> >
> >        * testsuite/g++.dg/ipa/pr45565.C: New test.
> >
> >
> > Index: icln/gcc/cgraphunit.c
> > ===================================================================
> > --- icln.orig/gcc/cgraphunit.c
> > +++ icln/gcc/cgraphunit.c
> > @@ -2113,7 +2113,8 @@ cgraph_materialize_clone (struct cgraph_
> >  }
> >
> >  /* If necessary, change the function declaration in the call statement
> > -   associated with E so that it corresponds to the edge callee.  */
> > +   associated with E so that it corresponds to the edge callee.  Return the new
> > +   statement or NULL if nothing was changed.  */
> >
> >  gimple
> >  cgraph_redirect_edge_call_stmt_to_callee (struct cgraph_edge *e)
> > @@ -2128,7 +2129,7 @@ cgraph_redirect_edge_call_stmt_to_callee
> >       || decl == e->callee->decl
> >       /* Don't update call from same body alias to the real function.  */
> >       || (decl && cgraph_get_node (decl) == cgraph_get_node (e->callee->decl)))
> > -    return e->call_stmt;
> > +    return NULL;
> >
> >  #ifdef ENABLE_CHECKING
> >   if (decl)
> > Index: icln/gcc/ipa-inline.c
> > ===================================================================
> > --- icln.orig/gcc/ipa-inline.c
> > +++ icln/gcc/ipa-inline.c
> > @@ -2161,7 +2161,8 @@ inline_transform (struct cgraph_node *no
> >
> >   for (e = node->callees; e; e = e->next_callee)
> >     {
> > -      cgraph_redirect_edge_call_stmt_to_callee (e);
> > +      if (cgraph_redirect_edge_call_stmt_to_callee (e))
> > +       todo = TODO_update_ssa;
> >       if (!e->inline_failed || warn_inline)
> >         inline_p = true;
> >     }
> > @@ -2169,7 +2170,7 @@ inline_transform (struct cgraph_node *no
> >   if (inline_p)
> >     {
> >       timevar_push (TV_INTEGRATION);
> > -      todo = optimize_inline_calls (current_function_decl);
> > +      todo |= optimize_inline_calls (current_function_decl);
> >       timevar_pop (TV_INTEGRATION);
> >     }
> >   cfun->always_inline_functions_inlined = true;
> > Index: icln/gcc/tree-inline.c
> > ===================================================================
> > --- icln.orig/gcc/tree-inline.c
> > +++ icln/gcc/tree-inline.c
> > @@ -1679,6 +1679,7 @@ copy_bb (copy_body_data *id, basic_block
> >                  if (edge)
> >                    {
> >                      int edge_freq = edge->frequency;
> > +                     gimple n_stmt;
> >                      edge = cgraph_clone_edge (edge, id->dst_node, stmt,
> >                                                gimple_uid (stmt),
> >                                                REG_BR_PROB_BASE, CGRAPH_FREQ_BASE,
> > @@ -1704,7 +1705,9 @@ copy_bb (copy_body_data *id, basic_block
> >                                   bb->frequency,
> >                                   copy_basic_block->frequency);
> >                        }
> > -                     stmt = cgraph_redirect_edge_call_stmt_to_callee (edge);
> > +                     n_stmt = cgraph_redirect_edge_call_stmt_to_callee (edge);
> > +                     if (n_stmt)
> > +                       stmt = n_stmt;
> >                    }
> >                  break;
> >
> > Index: icln/gcc/testsuite/g++.dg/ipa/pr45565.C
> > ===================================================================
> > --- /dev/null
> > +++ icln/gcc/testsuite/g++.dg/ipa/pr45565.C
> > @@ -0,0 +1,28 @@
> > +// { dg-do compile }
> > +// { dg-options "-O -fno-toplevel-reorder -fno-inline -fipa-cp -fipa-cp-clone -fkeep-inline-functions" }
> > +
> > +template < typename Derived > struct AnyMatrixBase
> > +{
> > +};
> > +
> > +struct Matrix Random ();
> > +
> > +struct Matrix:AnyMatrixBase < Matrix >
> > +{
> > +  void bar ()
> > +  {
> > +    throw;
> > +  }
> > +  void foo (Matrix other)
> > +  {
> > +    bar ();
> > +    Matrix (AnyMatrixBase < Matrix > (Random ()));
> > +  }
> > +  template
> > +    < typename OtherDerived > Matrix (AnyMatrixBase < OtherDerived > other)
> > +  {
> > +    foo (other);
> > +  }
> > +};
> > +
> > +Matrix x (Random ());
> >
Richard Guenther - Sept. 23, 2010, 12:37 p.m.
2010/9/23 Jan Hubicka <hubicka@ucw.cz>:
>> On Thu, Sep 23, 2010 at 11:16 AM, Martin Jambor <mjambor@suse.cz> wrote:
>> > Hi,
>> >
>> > the patch below fixes PR 45565 by updating SSA after inline transform
>> > even if we don't inline anything and only redirect an edge.  I'm a bit
>> > surprised this is necessary since we used to assert !need_ssa_update_p
>> > in these cases
>> > (http://gcc.gnu.org/ml/gcc-patches/2010-04/msg01012.html) ...but well,
>> > in inline transform this does not matter.
>> >
>> > Bootstrapped and tested on x86_64-linux.  OK for trunk?
>>
>> No.
>>
>> The issue seems to be that we call update_stmt during gsi_replace
>> with still the old fndecl on the stmt, and that old fndecl now became
>> pure and thus that update_stmt dropped the VDEF.  But the clone
>> with the skipped args is _not_ marked pure for some reason and
>> thus the following update_stmt gets back the VDEF but now requires
>> renaming it.
> Hmm, this sounds like bug in copying decl for new function.
> Definitly should be fixed..
> We are very lousy on copying and updating attributes when producing clones
> in general.

Are you sure it's not an ordering problem between pure-const and
whoever creates the fndecl clone?

Richard.

> Honza
>>
>> So I see a missed optimization here, and the PR can be fixed with
>>
>> Index: cgraphunit.c
>> ===================================================================
>> --- cgraphunit.c        (revision 164550)
>> +++ cgraphunit.c        (working copy)
>> @@ -2159,6 +2159,7 @@ cgraph_redirect_edge_call_stmt_to_callee
>>        new_stmt
>>         = gimple_call_copy_skip_args (e->call_stmt,
>>                                       e->callee->clone.combined_args_to_skip);
>> +      gimple_call_set_fndecl (new_stmt, e->callee->decl);
>>
>>        if (gimple_vdef (new_stmt)
>>           && TREE_CODE (gimple_vdef (new_stmt)) == SSA_NAME)
>> @@ -2168,10 +2169,11 @@ cgraph_redirect_edge_call_stmt_to_callee
>>        gsi_replace (&gsi, new_stmt, true);
>>      }
>>    else
>> -    new_stmt = e->call_stmt;
>> -
>> -  gimple_call_set_fndecl (new_stmt, e->callee->decl);
>> -  update_stmt (new_stmt);
>> +    {
>> +      new_stmt = e->call_stmt;
>> +      gimple_call_set_fndecl (new_stmt, e->callee->decl);
>> +      update_stmt (new_stmt);
>> +    }
>>
>>    cgraph_set_call_stmt_including_clones (e->caller, e->call_stmt, new_stmt);
>>
>>
>> instead.  Testing that now.
>>
>> Richard.
>>
>> > Thanks,
>> >
>> > Martin
>> >
>> >
>> > 2010-09-22  Martin Jambor  <mjambor@suse.cz>
>> >
>> >        PR middle-end/45565
>> >        * cgraphunit.c (cgraph_redirect_edge_call_stmt_to_callee): Return NULL
>> >        if nothing is changed.
>> >        * tree-inline.c (copy_bb): Handle that
>> >        cgraph_redirect_edge_call_stmt_to_callee can return NULL.
>> >        * ipa-inline.c (inline_transform): Update SSA if
>> >        cgraph_redirect_edge_call_stmt_to_callee returns non-NULL.
>> >
>> >        * testsuite/g++.dg/ipa/pr45565.C: New test.
>> >
>> >
>> > Index: icln/gcc/cgraphunit.c
>> > ===================================================================
>> > --- icln.orig/gcc/cgraphunit.c
>> > +++ icln/gcc/cgraphunit.c
>> > @@ -2113,7 +2113,8 @@ cgraph_materialize_clone (struct cgraph_
>> >  }
>> >
>> >  /* If necessary, change the function declaration in the call statement
>> > -   associated with E so that it corresponds to the edge callee.  */
>> > +   associated with E so that it corresponds to the edge callee.  Return the new
>> > +   statement or NULL if nothing was changed.  */
>> >
>> >  gimple
>> >  cgraph_redirect_edge_call_stmt_to_callee (struct cgraph_edge *e)
>> > @@ -2128,7 +2129,7 @@ cgraph_redirect_edge_call_stmt_to_callee
>> >       || decl == e->callee->decl
>> >       /* Don't update call from same body alias to the real function.  */
>> >       || (decl && cgraph_get_node (decl) == cgraph_get_node (e->callee->decl)))
>> > -    return e->call_stmt;
>> > +    return NULL;
>> >
>> >  #ifdef ENABLE_CHECKING
>> >   if (decl)
>> > Index: icln/gcc/ipa-inline.c
>> > ===================================================================
>> > --- icln.orig/gcc/ipa-inline.c
>> > +++ icln/gcc/ipa-inline.c
>> > @@ -2161,7 +2161,8 @@ inline_transform (struct cgraph_node *no
>> >
>> >   for (e = node->callees; e; e = e->next_callee)
>> >     {
>> > -      cgraph_redirect_edge_call_stmt_to_callee (e);
>> > +      if (cgraph_redirect_edge_call_stmt_to_callee (e))
>> > +       todo = TODO_update_ssa;
>> >       if (!e->inline_failed || warn_inline)
>> >         inline_p = true;
>> >     }
>> > @@ -2169,7 +2170,7 @@ inline_transform (struct cgraph_node *no
>> >   if (inline_p)
>> >     {
>> >       timevar_push (TV_INTEGRATION);
>> > -      todo = optimize_inline_calls (current_function_decl);
>> > +      todo |= optimize_inline_calls (current_function_decl);
>> >       timevar_pop (TV_INTEGRATION);
>> >     }
>> >   cfun->always_inline_functions_inlined = true;
>> > Index: icln/gcc/tree-inline.c
>> > ===================================================================
>> > --- icln.orig/gcc/tree-inline.c
>> > +++ icln/gcc/tree-inline.c
>> > @@ -1679,6 +1679,7 @@ copy_bb (copy_body_data *id, basic_block
>> >                  if (edge)
>> >                    {
>> >                      int edge_freq = edge->frequency;
>> > +                     gimple n_stmt;
>> >                      edge = cgraph_clone_edge (edge, id->dst_node, stmt,
>> >                                                gimple_uid (stmt),
>> >                                                REG_BR_PROB_BASE, CGRAPH_FREQ_BASE,
>> > @@ -1704,7 +1705,9 @@ copy_bb (copy_body_data *id, basic_block
>> >                                   bb->frequency,
>> >                                   copy_basic_block->frequency);
>> >                        }
>> > -                     stmt = cgraph_redirect_edge_call_stmt_to_callee (edge);
>> > +                     n_stmt = cgraph_redirect_edge_call_stmt_to_callee (edge);
>> > +                     if (n_stmt)
>> > +                       stmt = n_stmt;
>> >                    }
>> >                  break;
>> >
>> > Index: icln/gcc/testsuite/g++.dg/ipa/pr45565.C
>> > ===================================================================
>> > --- /dev/null
>> > +++ icln/gcc/testsuite/g++.dg/ipa/pr45565.C
>> > @@ -0,0 +1,28 @@
>> > +// { dg-do compile }
>> > +// { dg-options "-O -fno-toplevel-reorder -fno-inline -fipa-cp -fipa-cp-clone -fkeep-inline-functions" }
>> > +
>> > +template < typename Derived > struct AnyMatrixBase
>> > +{
>> > +};
>> > +
>> > +struct Matrix Random ();
>> > +
>> > +struct Matrix:AnyMatrixBase < Matrix >
>> > +{
>> > +  void bar ()
>> > +  {
>> > +    throw;
>> > +  }
>> > +  void foo (Matrix other)
>> > +  {
>> > +    bar ();
>> > +    Matrix (AnyMatrixBase < Matrix > (Random ()));
>> > +  }
>> > +  template
>> > +    < typename OtherDerived > Matrix (AnyMatrixBase < OtherDerived > other)
>> > +  {
>> > +    foo (other);
>> > +  }
>> > +};
>> > +
>> > +Matrix x (Random ());
>> >
>
Jan Hubicka - Sept. 23, 2010, 10:18 p.m.
> 2010/9/23 Jan Hubicka <hubicka@ucw.cz>:
> >> On Thu, Sep 23, 2010 at 11:16 AM, Martin Jambor <mjambor@suse.cz> wrote:
> >> > Hi,
> >> >
> >> > the patch below fixes PR 45565 by updating SSA after inline transform
> >> > even if we don't inline anything and only redirect an edge.  I'm a bit
> >> > surprised this is necessary since we used to assert !need_ssa_update_p
> >> > in these cases
> >> > (http://gcc.gnu.org/ml/gcc-patches/2010-04/msg01012.html) ...but well,
> >> > in inline transform this does not matter.
> >> >
> >> > Bootstrapped and tested on x86_64-linux.  OK for trunk?
> >>
> >> No.
> >>
> >> The issue seems to be that we call update_stmt during gsi_replace
> >> with still the old fndecl on the stmt, and that old fndecl now became
> >> pure and thus that update_stmt dropped the VDEF.  But the clone
> >> with the skipped args is _not_ marked pure for some reason and
> >> thus the following update_stmt gets back the VDEF but now requires
> >> renaming it.
> > Hmm, this sounds like bug in copying decl for new function.
> > Definitly should be fixed..
> > We are very lousy on copying and updating attributes when producing clones
> > in general.
> 
> Are you sure it's not an ordering problem between pure-const and
> whoever creates the fndecl clone?
Well, when you clone function before pure const, pure const should just see the
clone too and also figure out it is pure.  When you clone after, you should copy
the flag.  So in theory this ought to work.

Honza

Patch

Index: cgraphunit.c
===================================================================
--- cgraphunit.c        (revision 164550)
+++ cgraphunit.c        (working copy)
@@ -2159,6 +2159,7 @@  cgraph_redirect_edge_call_stmt_to_callee
       new_stmt
        = gimple_call_copy_skip_args (e->call_stmt,
                                      e->callee->clone.combined_args_to_skip);
+      gimple_call_set_fndecl (new_stmt, e->callee->decl);

       if (gimple_vdef (new_stmt)
          && TREE_CODE (gimple_vdef (new_stmt)) == SSA_NAME)
@@ -2168,10 +2169,11 @@  cgraph_redirect_edge_call_stmt_to_callee
       gsi_replace (&gsi, new_stmt, true);
     }
   else
-    new_stmt = e->call_stmt;
-
-  gimple_call_set_fndecl (new_stmt, e->callee->decl);
-  update_stmt (new_stmt);
+    {
+      new_stmt = e->call_stmt;
+      gimple_call_set_fndecl (new_stmt, e->callee->decl);
+      update_stmt (new_stmt);
+    }

   cgraph_set_call_stmt_including_clones (e->caller, e->call_stmt, new_stmt);