| Message ID | 20160211160633.GB3017@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <jakub@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 > >
--- 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 (); +}