On Thu, 4 Apr 2013, Jakub Jelinek wrote:
> Hi!
>
> The vt3.C testcase (from PR34949) ICEd, because sink_clobbers sunk a
> MEM_REF[SSA_NAME] clobber from a landing pad which that SSA_NAME definition
> dominated to an outer one which wasn't dominated by the definition.
> As the ehcleanup nor ehdisp passes keep dominance info current and perform
> changes that invalidate it, I can't unfortunately do cheaply a
> dominated_by_p check, so the patch just throws away all MEM_REF[SSA_NAME]
> clobbers if SSA_NAME isn't a default def which is valid everywhere. As
> sink_clobbers is only done on otherwise empty bb's and typically the
> clobbers are preceeded by some stores which are to be DSEd if unneeded
> and after DSEing aren't really needed anymore, this doesn't seem to hurt
> much.
> The patch also improves optimize_clobbers, so that it only removes any
> clobbers if the bb is actually empty (except for clobbers, resx, maybe debug
> stmts or __builtin_stack_restore), that way needed clobbers are kept around
> until they are used by DSE.
> Also, MEM_REF[SSA_NAME] clobbers aren't useful very late in the optimization
> pipeline, but could cause some SSA_NAMEs to be considered unnecessarily live
> (especially if they are considered live across EH edges it is undesirable),
> such clobbers are mainly useful during DSE1/DSE2, but at expansion are
> completely ignored (unlike VAR_DECL clobbers, which are also used for the
> stack layout decisions), so the patch removes all those MEM_REF[SSA_NAME]
> clobbers shortly after dse2 (in fab pass).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, on libstdc++ I saw
> some code size reduction with the patch.
>
> Ok for trunk?
Ok.
Thanks,
Richard.
> 2013-04-04 Jakub Jelinek <ja...@redhat.com>
>
> PR c++/34949
> PR c++/50243
> * tree-eh.c (optimize_clobbers): Only remove clobbers if bb doesn't
> contain anything but clobbers, at most one __builtin_stack_restore,
> optionally debug stmts and final resx, and if it has at least one
> incoming EH edge. Don't check for SSA_NAME on LHS of a clobber.
> (sink_clobbers): Don't check for SSA_NAME on LHS of a clobber.
> Instead of moving clobbers with MEM_REF LHS with SSA_NAME address
> which isn't defaut definition, remove them.
> (unsplit_eh, cleanup_empty_eh): Use single_{pred,succ}_{p,edge}
> instead of EDGE_COUNT comparisons or EDGE_{PRED,SUCC}.
> * tree-ssa-ccp.c (execute_fold_all_builtins): Remove clobbers
> with MEM_REF LHS with SSA_NAME address.
>
> * g++.dg/opt/vt3.C: New test.
> * g++.dg/opt/vt4.C: New test.
>
> --- gcc/tree-eh.c.jj 2013-03-26 10:03:55.000000000 +0100
> +++ gcc/tree-eh.c 2013-04-04 13:44:27.718982776 +0200
> @@ -3230,14 +3230,48 @@ static void
> optimize_clobbers (basic_block bb)
> {
> gimple_stmt_iterator gsi = gsi_last_bb (bb);
> + bool any_clobbers = false;
> + bool seen_stack_restore = false;
> + edge_iterator ei;
> + edge e;
> +
> + /* Only optimize anything if the bb contains at least one clobber,
> + ends with resx (checked by caller), optionally contains some
> + debug stmts or labels, or at most one __builtin_stack_restore
> + call, and has an incoming EH edge. */
> for (gsi_prev (&gsi); !gsi_end_p (gsi); gsi_prev (&gsi))
> {
> gimple stmt = gsi_stmt (gsi);
> if (is_gimple_debug (stmt))
> continue;
> - if (!gimple_clobber_p (stmt)
> - || TREE_CODE (gimple_assign_lhs (stmt)) == SSA_NAME)
> - return;
> + if (gimple_clobber_p (stmt))
> + {
> + any_clobbers = true;
> + continue;
> + }
> + if (!seen_stack_restore
> + && gimple_call_builtin_p (stmt, BUILT_IN_STACK_RESTORE))
> + {
> + seen_stack_restore = true;
> + continue;
> + }
> + if (gimple_code (stmt) == GIMPLE_LABEL)
> + break;
> + return;
> + }
> + if (!any_clobbers)
> + return;
> + FOR_EACH_EDGE (e, ei, bb->preds)
> + if (e->flags & EDGE_EH)
> + break;
> + if (e == NULL)
> + return;
> + gsi = gsi_last_bb (bb);
> + for (gsi_prev (&gsi); !gsi_end_p (gsi); gsi_prev (&gsi))
> + {
> + gimple stmt = gsi_stmt (gsi);
> + if (!gimple_clobber_p (stmt))
> + continue;
> unlink_stmt_vdef (stmt);
> gsi_remove (&gsi, true);
> release_defs (stmt);
> @@ -3278,8 +3312,7 @@ sink_clobbers (basic_block bb)
> continue;
> if (gimple_code (stmt) == GIMPLE_LABEL)
> break;
> - if (!gimple_clobber_p (stmt)
> - || TREE_CODE (gimple_assign_lhs (stmt)) == SSA_NAME)
> + if (!gimple_clobber_p (stmt))
> return 0;
> any_clobbers = true;
> }
> @@ -3292,11 +3325,27 @@ sink_clobbers (basic_block bb)
> for (gsi_prev (&gsi); !gsi_end_p (gsi); gsi_prev (&gsi))
> {
> gimple stmt = gsi_stmt (gsi);
> + tree lhs;
> if (is_gimple_debug (stmt))
> continue;
> if (gimple_code (stmt) == GIMPLE_LABEL)
> break;
> unlink_stmt_vdef (stmt);
> + lhs = gimple_assign_lhs (stmt);
> + /* Unfortunately we don't have dominance info updated at this
> + point, so checking if
> + dominated_by_p (CDI_DOMINATORS, succbb,
> + gimple_bb (SSA_NAME_DEF_STMT (TREE_OPERAND (lhs, 0)))
> + would be too costly. Thus, avoid sinking any clobbers that
> + refer to non-(D) SSA_NAMEs. */
> + if (TREE_CODE (lhs) == MEM_REF
> + && TREE_CODE (TREE_OPERAND (lhs, 0)) == SSA_NAME
> + && !SSA_NAME_IS_DEFAULT_DEF (TREE_OPERAND (lhs, 0)))
> + {
> + gsi_remove (&gsi, true);
> + release_defs (stmt);
> + continue;
> + }
> gsi_remove (&gsi, false);
> /* Trigger the operand scanner to cause renaming for virtual
> operands for this statement.
> @@ -3737,10 +3786,10 @@ unsplit_eh (eh_landing_pad lp)
> edge e_in, e_out;
>
> /* Quickly check the edge counts on BB for singularity. */
> - if (EDGE_COUNT (bb->preds) != 1 || EDGE_COUNT (bb->succs) != 1)
> + if (!single_pred_p (bb) || !single_succ_p (bb))
> return false;
> - e_in = EDGE_PRED (bb, 0);
> - e_out = EDGE_SUCC (bb, 0);
> + e_in = single_pred_edge (bb);
> + e_out = single_succ_edge (bb);
>
> /* Input edge must be EH and output edge must be normal. */
> if ((e_in->flags & EDGE_EH) == 0 || (e_out->flags & EDGE_EH) != 0)
> @@ -4142,7 +4191,7 @@ cleanup_empty_eh (eh_landing_pad lp)
> e_out = NULL;
> break;
> case 1:
> - e_out = EDGE_SUCC (bb, 0);
> + e_out = single_succ_edge (bb);
> break;
> default:
> return false;
> --- gcc/tree-ssa-ccp.c.jj 2013-02-20 18:40:49.000000000 +0100
> +++ gcc/tree-ssa-ccp.c 2013-04-04 14:01:08.926335910 +0200
> @@ -2396,6 +2396,21 @@ execute_fold_all_builtins (void)
>
> if (gimple_code (stmt) != GIMPLE_CALL)
> {
> + /* Remove all *ssaname_N ={v} {CLOBBER}; stmts,
> + after the last GIMPLE DSE they aren't needed and might
> + unnecessarily keep the SSA_NAMEs live. */
> + if (gimple_clobber_p (stmt))
> + {
> + tree lhs = gimple_assign_lhs (stmt);
> + if (TREE_CODE (lhs) == MEM_REF
> + && TREE_CODE (TREE_OPERAND (lhs, 0)) == SSA_NAME)
> + {
> + unlink_stmt_vdef (stmt);
> + gsi_remove (&i, true);
> + release_defs (stmt);
> + continue;
> + }
> + }
> gsi_next (&i);
> continue;
> }
> --- gcc/testsuite/g++.dg/opt/vt3.C.jj 2013-04-04 14:33:12.190484331 +0200
> +++ gcc/testsuite/g++.dg/opt/vt3.C 2013-04-04 14:32:19.000000000 +0200
> @@ -0,0 +1,43 @@
> +// PR c++/34949
> +// { dg-do compile }
> +// { dg-options "-O3" }
> +
> +struct E {};
> +struct A
> +{
> + virtual void a (void *) = 0;
> +};
> +struct B
> +{
> + virtual ~B () {};
> + unsigned int b1;
> + E **b2;
> + A *b3;
> +};
> +struct C : public B
> +{
> + ~C ();
> +};
> +C::~C ()
> +{
> + for (unsigned int i = 0; i < b1; i++)
> + b3->a (b2);
> +}
> +struct D
> +{
> + ~D () {}
> + C d;
> +};
> +struct F { virtual ~F () {}; };
> +struct G { void g (); };
> +struct H : public F
> +{
> + virtual ~H ();
> + D *h1;
> + G *h2;
> +};
> +H::~H ()
> +{
> + h2->g ();
> + delete h1;
> +}
> --- gcc/testsuite/g++.dg/opt/vt4.C.jj 2013-04-04 14:33:35.684354321 +0200
> +++ gcc/testsuite/g++.dg/opt/vt4.C 2013-04-04 14:33:44.245304179 +0200
> @@ -0,0 +1,31 @@
> +// PR c++/50243
> +// { dg-do compile }
> +// { dg-options "-O" }
> +// { dg-final { scan-assembler-not "_ZTV.A" } }
> +
> +void foo ();
> +
> +struct A
> +{
> + ~A () { }
> + virtual void a () = 0;
> + virtual void b () = 0;
> + virtual void c () = 0;
> +};
> +
> +struct B : public A
> +{
> + ~B () { foo (); }
> + void a () { foo (); }
> + void b () { foo (); }
> + void c () { delete this; }
> +};
> +
> +void
> +test ()
> +{
> + A *y = new B ();
> + y->a ();
> + y->b ();
> + y->c ();
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend
