On Thu, 11 Feb 2016, Jakub Jelinek wrote:
> Hi!
>
> While the cgraph versioning code is able to deal with clobber stmts that
> refer to SSA_NAMEs set by basic blocks not split into the versioned
> function, and similarly with debug stmts (clobbers refererring to those
> are removed and debug stmts reset), on the main part that doesn't happen,
> because ipa-split makes the split blocks unreachable by jumping around them
> and thus if SSA_NAMEs set in the split basic blocks are referenced
> in return_bb in statements we intentionally ignored for the analysis (debug
> stmts and clobber stmts), we either end up with weird stuff (set debug stmts
> to debug temporary that is set in the basic blocks that are removed as
> dead), or for clobbers ICE.
>
> Richard has tried to deal with this by throwing away the return_bb in
> certain cases in the main part (forcing recreation of it), but that can
> throw valuable statements that we could have kept (debug stmts or e.g. decl
> clobbers or clobbers that don't need SSA_NAMEs from the split blocks),
> and doesn't work e.g. on the 1st and 3rd testcase below anyway.
>
> So, this patch reverts the main_part_return_p and instead does
> what is needed for those ignored stmts in return_bb:
> 1) SSA_NAME references to retval in both debug stmts and clobber stmts
> are replaced with the lhs of the call to *.part.*
> 2) debug stmts referencing other SSA_NAMEs set in the split bbs are reset
> 3) clobber stmts referencing other SSA_NAMEs set in the split bbs are removed
>
> The testcase hopefully cover all the interesting cases (return_bb copied
> from main part (where it is kept) to split part too, and with debug stmts
> and clobbers that refer to SSA_NAMEs set in main or split part or retval).
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok.
Thanks,
Richard.
> 2016-02-11 Jakub Jelinek <ja...@redhat.com>
>
> PR ipa/68672
> * ipa-split.c (split_function): Don't compute/use main_part_return_p.
> Compute retval and retbnd early in all cases if split_part_return_p
> and return_bb is not EXIT. Remove all clobber stmts and reset
> all debug stmts that refer to SSA_NAMEs defined in split part,
> except if it is retval, in that case replace the old retval with the
> lhs of the call to the split part.
>
> * g++.dg/ipa/pr68672-1.C: New test.
> * g++.dg/ipa/pr68672-2.C: New test.
> * g++.dg/ipa/pr68672-3.C: New test.
>
> --- gcc/ipa-split.c.jj 2016-02-11 10:50:52.888220581 +0100
> +++ gcc/ipa-split.c 2016-02-11 12:46:15.975777652 +0100
> @@ -1244,28 +1244,13 @@ split_function (basic_block return_bb, s
> args_to_pass.safe_push (arg);
> }
>
> - /* See if the split function or the main part will return. */
> - bool main_part_return_p = false;
> + /* See if the split function will return. */
> bool split_part_return_p = false;
> FOR_EACH_EDGE (e, ei, return_bb->preds)
> {
> if (bitmap_bit_p (split_point->split_bbs, e->src->index))
> split_part_return_p = true;
> - else
> - main_part_return_p = true;
> }
> - /* The main part also returns if we split on a fallthru edge
> - and the split part returns. */
> - if (split_part_return_p)
> - FOR_EACH_EDGE (e, ei, split_point->entry_bb->preds)
> - {
> - if (! bitmap_bit_p (split_point->split_bbs, e->src->index)
> - && single_succ_p (e->src))
> - {
> - main_part_return_p = true;
> - break;
> - }
> - }
>
> /* Add return block to what will become the split function.
> We do not return; no return block is needed. */
> @@ -1279,8 +1264,8 @@ split_function (basic_block return_bb, s
> FIXME: Once we are able to change return type, we should change function
> to return void instead of just outputting function with undefined return
> value. For structures this affects quality of codegen. */
> - else if (!split_point->split_part_set_retval
> - && (retval = find_retval (return_bb)))
> + else if ((retval = find_retval (return_bb))
> + && !split_point->split_part_set_retval)
> {
> bool redirected = true;
> basic_block new_return_bb = create_basic_block (NULL, 0, return_bb);
> @@ -1308,12 +1293,10 @@ split_function (basic_block return_bb, s
> }
> /* When we pass around the value, use existing return block. */
> else
> - bitmap_set_bit (split_point->split_bbs, return_bb->index);
> -
> - /* If the main part doesn't return pretend the return block wasn't
> - found for all of the following. */
> - if (! main_part_return_p)
> - return_bb = EXIT_BLOCK_PTR_FOR_FN (cfun);
> + {
> + bitmap_set_bit (split_point->split_bbs, return_bb->index);
> + retbnd = find_retbnd (return_bb);
> + }
>
> /* If RETURN_BB has virtual operand PHIs, they must be removed and the
> virtual operand marked for renaming as we change the CFG in a way that
> @@ -1382,6 +1365,44 @@ split_function (basic_block return_bb, s
> DECL_FUNCTION_CODE (node->decl) = (enum built_in_function) 0;
> }
>
> + /* If return_bb contains any clobbers that refer to SSA_NAMEs
> + set in the split part, remove them. Also reset debug stmts that
> + refer to SSA_NAMEs set in the split part. */
> + if (return_bb != EXIT_BLOCK_PTR_FOR_FN (cfun))
> + {
> + gimple_stmt_iterator gsi = gsi_start_bb (return_bb);
> + while (!gsi_end_p (gsi))
> + {
> + tree op;
> + ssa_op_iter iter;
> + gimple *stmt = gsi_stmt (gsi);
> + bool remove = false;
> + if (gimple_clobber_p (stmt) || is_gimple_debug (stmt))
> + FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE)
> + {
> + basic_block bb = gimple_bb (SSA_NAME_DEF_STMT (op));
> + if (op != retval
> + && bb
> + && bb != return_bb
> + && bitmap_bit_p (split_point->split_bbs, bb->index))
> + {
> + if (is_gimple_debug (stmt))
> + {
> + gimple_debug_bind_reset_value (stmt);
> + update_stmt (stmt);
> + }
> + else
> + remove = true;
> + break;
> + }
> + }
> + if (remove)
> + gsi_remove (&gsi, true);
> + else
> + gsi_next (&gsi);
> + }
> + }
> +
> /* If the original function is instrumented then it's
> part is also instrumented. */
> if (with_bounds)
> @@ -1499,9 +1520,7 @@ split_function (basic_block return_bb, s
> return value into and put call just before it. */
> if (return_bb != EXIT_BLOCK_PTR_FOR_FN (cfun))
> {
> - real_retval = retval = find_retval (return_bb);
> - retbnd = find_retbnd (return_bb);
> -
> + real_retval = retval;
> if (real_retval && split_point->split_part_set_retval)
> {
> gphi_iterator psi;
> @@ -1545,6 +1564,28 @@ split_function (basic_block return_bb, s
> break;
> }
> update_stmt (gsi_stmt (bsi));
> + /* Also adjust clobbers and debug stmts in return_bb. */
> + for (bsi = gsi_start_bb (return_bb); !gsi_end_p (bsi);
> + gsi_next (&bsi))
> + {
> + gimple *stmt = gsi_stmt (bsi);
> + if (gimple_clobber_p (stmt)
> + || is_gimple_debug (stmt))
> + {
> + ssa_op_iter iter;
> + use_operand_p use_p;
> + bool update = false;
> + FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter,
> + SSA_OP_USE)
> + if (USE_FROM_PTR (use_p) == real_retval)
> + {
> + SET_USE (use_p, retval);
> + update = true;
> + }
> + if (update)
> + update_stmt (stmt);
> + }
> + }
> }
>
> /* Replace retbnd with new one. */
> --- gcc/testsuite/g++.dg/ipa/pr68672-1.C.jj 2016-02-11 12:31:27.344497187
> +0100
> +++ gcc/testsuite/g++.dg/ipa/pr68672-1.C 2016-02-11 12:31:01.000000000
> +0100
> @@ -0,0 +1,20 @@
> +// PR ipa/68672
> +// { dg-do compile }
> +// { dg-options "-O -finline-small-functions -fpartial-inlining
> --param=partial-inlining-entry-probability=100" }
> +
> +void f2 (void *);
> +void *a;
> +struct C { virtual void m1 (); };
> +struct D { C *m2 () { if (a) __builtin_abort (); } };
> +D f1 ();
> +struct E { int e; ~E () { if (e) f2 (&e); } };
> +E *b;
> +struct I { virtual void m3 (); };
> +
> +void
> +I::m3 ()
> +{
> + if (a)
> + f1 ().m2 ()->m1 ();
> + b->~E ();
> +}
> --- gcc/testsuite/g++.dg/ipa/pr68672-2.C.jj 2016-02-11 12:33:50.744438948
> +0100
> +++ gcc/testsuite/g++.dg/ipa/pr68672-2.C 2016-02-11 12:32:50.000000000
> +0100
> @@ -0,0 +1,54 @@
> +// PR ipa/68672
> +// { dg-do compile }
> +// { dg-options "-O3 --param=partial-inlining-entry-probability=100 -g" }
> +
> +struct S { ~S () {} };
> +S *a;
> +int *b;
> +void bar ();
> +void baz ();
> +void fn (int *);
> +
> +static int
> +foo ()
> +{
> + S *c = a;
> + if (c)
> + {
> + bar ();
> + if (a)
> + __builtin_abort ();
> + baz ();
> + }
> + int p = *b;
> + if (p)
> + {
> + fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn
> (b); fn (b);
> + fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn
> (b); fn (b);
> + fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn
> (b); fn (b);
> + fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn
> (b); fn (b);
> + }
> + c->~S ();
> + int q = 2 * p;
> + int r = 3 * q;
> + S *d = c;
> + return p;
> +}
> +
> +void
> +use1 ()
> +{
> + foo ();
> +}
> +
> +void
> +use2 ()
> +{
> + foo ();
> +}
> +
> +void
> +use3 ()
> +{
> + foo ();
> +}
> --- gcc/testsuite/g++.dg/ipa/pr68672-3.C.jj 2016-02-11 12:34:02.374272024
> +0100
> +++ gcc/testsuite/g++.dg/ipa/pr68672-3.C 2016-02-11 12:34:22.337985482
> +0100
> @@ -0,0 +1,57 @@
> +// PR ipa/68672
> +// { dg-do compile }
> +// { dg-options "-O3 --param=partial-inlining-entry-probability=100 -g" }
> +
> +struct S { ~S () {} };
> +S *a, *e;
> +int *b;
> +void bar ();
> +void baz ();
> +void fn (int *);
> +void fn2 (S *);
> +
> +static int
> +foo ()
> +{
> + S *c = a;
> + if (c)
> + {
> + bar ();
> + if (a)
> + __builtin_abort ();
> + baz ();
> + }
> + int p = *b;
> + S *f = e;
> + if (p)
> + {
> + fn2 (f);
> + fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn
> (b); fn (b);
> + fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn
> (b); fn (b);
> + fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn
> (b); fn (b);
> + fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn (b); fn
> (b); fn (b);
> + }
> + f->~S ();
> + int q = 2 * p;
> + int r = 3 * q;
> + S *d = c;
> + return p;
> +}
> +
> +void
> +use1 ()
> +{
> + foo ();
> +}
> +
> +void
> +use2 ()
> +{
> + foo ();
> +}
> +
> +void
> +use3 ()
> +{
> + foo ();
> +}
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
