On Fri, Jun 3, 2011 at 3:55 PM, Jakub Jelinek <ja...@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 <ja...@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
>
