Fix fold_stmt ICE (PR c++/49264)

Hi!

If memcpy is folded into an assignment, that assignment can be for C++
folded into nothing (if it is copying of 1 byte from or to empty C++ class).
gimple-fold.c was changed to handle that case in some spots, but not all,
particularly if that memcpy is the last stmt in a basic block (which can
happen e.g. during inlining), gsi_stmt (*gsi) in fold_stmt_1 will ICE.
The gimple-fold.c hunks fix this (and also make sure that even when it
isn't the last stmt in a bb, we won't try to fold lhs of next stmt
if it folded/gimplified into nothing).  With that fix the testcase ICEs
shortly afterwards, the tree-inline.c hunk is supposed to fix that.
I'd use cgraph_remove_edge there, but I'd have to handle all clones too,
so I'm calling cgraph_update_edges_for_call_stmt with a dummy stmt
instead (perhaps NULL could be used instead with adjustment of the
cgraph_update_edges_for_call_stmt{,_node} routines?).
The cgraph.c change is just in case the edge isn't there, we'd do useless
work and crash while doing that.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.6?

2011-06-03  Jakub Jelinek  <jakub@redhat.com>

	PR c++/49264
	* gimple-fold.c (fold_stmt_1): Don't try to fold *& on the lhs
	if stmt folded into nothing.
	* tree-inline.c (fold_marked_statements): If a builtin at the
	end of a bb folded into nothing, just update cgraph edges
	and move to next bb.
	* cgraph.c (cgraph_update_edges_for_call_stmt_node): Don't compute
	count and frequency if new_call is NULL.

	* g++.dg/opt/pr49264.C: New test.


	Jakub

On Fri, Jun 3, 2011 at 3:55 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> If memcpy is folded into an assignment, that assignment can be for C++
> folded into nothing (if it is copying of 1 byte from or to empty C++ class).
> gimple-fold.c was changed to handle that case in some spots, but not all,
> particularly if that memcpy is the last stmt in a basic block (which can
> happen e.g. during inlining), gsi_stmt (*gsi) in fold_stmt_1 will ICE.
> The gimple-fold.c hunks fix this (and also make sure that even when it
> isn't the last stmt in a bb, we won't try to fold lhs of next stmt
> if it folded/gimplified into nothing).  With that fix the testcase ICEs
> shortly afterwards, the tree-inline.c hunk is supposed to fix that.
> I'd use cgraph_remove_edge there, but I'd have to handle all clones too,
> so I'm calling cgraph_update_edges_for_call_stmt with a dummy stmt
> instead (perhaps NULL could be used instead with adjustment of the
> cgraph_update_edges_for_call_stmt{,_node} routines?).
> The cgraph.c change is just in case the edge isn't there, we'd do useless
> work and crash while doing that.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.6?

Ok, but ... Honza, can you suggest sth better for ...

> 2011-06-03  Jakub Jelinek  <jakub@redhat.com>
>
>        PR c++/49264
>        * gimple-fold.c (fold_stmt_1): Don't try to fold *& on the lhs
>        if stmt folded into nothing.
>        * tree-inline.c (fold_marked_statements): If a builtin at the
>        end of a bb folded into nothing, just update cgraph edges
>        and move to next bb.
>        * cgraph.c (cgraph_update_edges_for_call_stmt_node): Don't compute
>        count and frequency if new_call is NULL.
>
>        * g++.dg/opt/pr49264.C: New test.
>
> --- gcc/gimple-fold.c.jj        2011-05-24 23:34:28.000000000 +0200
> +++ gcc/gimple-fold.c   2011-06-03 09:13:34.000000000 +0200
> @@ -1577,6 +1577,11 @@ fold_stmt_1 (gimple_stmt_iterator *gsi,
>   bool changed = false;
>   gimple stmt = gsi_stmt (*gsi);
>   unsigned i;
> +  gimple_stmt_iterator gsinext = *gsi;
> +  gimple next_stmt;
> +
> +  gsi_next (&gsinext);
> +  next_stmt = gsi_end_p (gsinext) ? NULL : gsi_stmt (gsinext);
>
>   /* Fold the main computation performed by the statement.  */
>   switch (gimple_code (stmt))
> @@ -1665,10 +1670,19 @@ fold_stmt_1 (gimple_stmt_iterator *gsi,
>     default:;
>     }
>
> +  /* If stmt folds into nothing and it was the last stmt in a bb,
> +     don't call gsi_stmt.  */
> +  if (gsi_end_p (*gsi))
> +    {
> +      gcc_assert (next_stmt == NULL);
> +      return changed;
> +    }
> +
>   stmt = gsi_stmt (*gsi);
>
> -  /* Fold *& on the lhs.  */
> -  if (gimple_has_lhs (stmt))
> +  /* Fold *& on the lhs.  Don't do this if stmt folded into nothing,
> +     as we'd changing the next stmt.  */
> +  if (gimple_has_lhs (stmt) && stmt != next_stmt)
>     {
>       tree lhs = gimple_get_lhs (stmt);
>       if (lhs && REFERENCE_CLASS_P (lhs))
> --- gcc/tree-inline.c.jj        2011-06-02 10:15:20.000000000 +0200
> +++ gcc/tree-inline.c   2011-06-03 09:29:15.000000000 +0200
> @@ -4108,6 +4108,14 @@ fold_marked_statements (int first, struc
>                  if (fold_stmt (&gsi))
>                    {
>                      gimple new_stmt;
> +                     /* If a builtin at the end of a bb folded into nothing,
> +                        the following loop won't work.  */
> +                     if (gsi_end_p (gsi))
> +                       {
> +                         cgraph_update_edges_for_call_stmt (old_stmt, old_decl,
> +                                                            gimple_build_nop ());

This?  Esp. I don't like the gimple_build_nop () here too much.

Thanks,
Richard.

> +                         break;
> +                       }
>                      if (gsi_end_p (i2))
>                        i2 = gsi_start_bb (BASIC_BLOCK (first));
>                      else
> --- gcc/cgraph.c.jj     2011-05-11 19:39:03.000000000 +0200
> +++ gcc/cgraph.c        2011-06-03 09:40:02.000000000 +0200
> @@ -1277,7 +1277,7 @@ cgraph_update_edges_for_call_stmt_node (
>          frequency = e->frequency;
>          cgraph_remove_edge (e);
>        }
> -      else
> +      else if (new_call)
>        {
>          /* We are seeing new direct call; compute profile info based on BB.  */
>          basic_block bb = gimple_bb (new_stmt);
> --- gcc/testsuite/g++.dg/opt/pr49264.C.jj       2011-06-03 09:35:50.000000000 +0200
> +++ gcc/testsuite/g++.dg/opt/pr49264.C  2011-06-03 08:50:33.000000000 +0200
> @@ -0,0 +1,19 @@
> +// PR c++/49264
> +// { dg-do compile }
> +// { dg-options "-O2" }
> +
> +struct B { };
> +struct A { char a[sizeof (B) + 1]; } a;
> +
> +static inline void
> +foo (const B &b)
> +{
> +  __builtin_memcpy (&a, &b, sizeof (b));
> +}
> +
> +void
> +bar ()
> +{
> +  B c;
> +  foo (c);
> +}
>
>        Jakub
>

On Mon, Jun 06, 2011 at 11:30:19AM +0200, Richard Guenther wrote:
> On Fri, Jun 3, 2011 at 3:55 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> > --- gcc/tree-inline.c.jj        2011-06-02 10:15:20.000000000 +0200
> > +++ gcc/tree-inline.c   2011-06-03 09:29:15.000000000 +0200
> > @@ -4108,6 +4108,14 @@ fold_marked_statements (int first, struc
> >                  if (fold_stmt (&gsi))
> >                    {
> >                      gimple new_stmt;
> > +                     /* If a builtin at the end of a bb folded into nothing,
> > +                        the following loop won't work.  */
> > +                     if (gsi_end_p (gsi))
> > +                       {
> > +                         cgraph_update_edges_for_call_stmt (old_stmt, old_decl,
> > +                                                            gimple_build_nop ());
> 
> This?  Esp. I don't like the gimple_build_nop () here too much.

Yeah, I've talked about it in my patch comment.
E.g. cgraph_update_edges_for_call_stmt could accept NULL as new_stmt, or we
could add e.g.

void
cgraph_remove_edges_for_call_stmt (gimple old_stmt)
{
  struct cgraph_node *orig = cgraph_get_node (cfun->decl);
  struct cgraph_node *node;
  struct cgraph_edge *e;

  gcc_checking_assert (orig);
  e = cgraph_edge (orig, old_stmt);
  if (e)
    cgraph_remove_edge (e);
  if (orig->clones)
    for (node = orig->clones; node != orig; )
      {
	e = cgraph_edge (node, old_stmt);
	if (e)
	  cgraph_remove_edge (e);
	if (node->clones)
	  node = node->clones;
	else if (node->next_sibling_clone)
	  node = node->next_sibling_clone;
	else
	  {
	    while (node != orig && !node->next_sibling_clone)
	      node = node->clone_of;
	    if (node != orig)
	      node = node->next_sibling_clone;
	  }
      }
}

I think NULL new_stmt would have the advantage that we wouldn't duplicate
the complex code looping through all kinds of clones.

	Jakub

On Mon, Jun 6, 2011 at 1:19 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Jun 06, 2011 at 11:30:19AM +0200, Richard Guenther wrote:
>> On Fri, Jun 3, 2011 at 3:55 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > --- gcc/tree-inline.c.jj        2011-06-02 10:15:20.000000000 +0200
>> > +++ gcc/tree-inline.c   2011-06-03 09:29:15.000000000 +0200
>> > @@ -4108,6 +4108,14 @@ fold_marked_statements (int first, struc
>> >                  if (fold_stmt (&gsi))
>> >                    {
>> >                      gimple new_stmt;
>> > +                     /* If a builtin at the end of a bb folded into nothing,
>> > +                        the following loop won't work.  */
>> > +                     if (gsi_end_p (gsi))
>> > +                       {
>> > +                         cgraph_update_edges_for_call_stmt (old_stmt, old_decl,
>> > +                                                            gimple_build_nop ());
>>
>> This?  Esp. I don't like the gimple_build_nop () here too much.
>
> Yeah, I've talked about it in my patch comment.
> E.g. cgraph_update_edges_for_call_stmt could accept NULL as new_stmt, or we
> could add e.g.
>
> void
> cgraph_remove_edges_for_call_stmt (gimple old_stmt)
> {
>  struct cgraph_node *orig = cgraph_get_node (cfun->decl);
>  struct cgraph_node *node;
>  struct cgraph_edge *e;
>
>  gcc_checking_assert (orig);
>  e = cgraph_edge (orig, old_stmt);
>  if (e)
>    cgraph_remove_edge (e);
>  if (orig->clones)
>    for (node = orig->clones; node != orig; )
>      {
>        e = cgraph_edge (node, old_stmt);
>        if (e)
>          cgraph_remove_edge (e);
>        if (node->clones)
>          node = node->clones;
>        else if (node->next_sibling_clone)
>          node = node->next_sibling_clone;
>        else
>          {
>            while (node != orig && !node->next_sibling_clone)
>              node = node->clone_of;
>            if (node != orig)
>              node = node->next_sibling_clone;
>          }
>      }
> }
>
> I think NULL new_stmt would have the advantage that we wouldn't duplicate
> the complex code looping through all kinds of clones.

Yeah, I'd prefer that variant.  Honza?

Richard.

>        Jakub
>