On Thu, Jul 28, 2011 at 6:52 PM, Martin Jambor <mjam...@suse.cz> wrote: > Hi, > > pass_split_functions is happy to split functions which have type > attributes but cannot update them if the new clone has in any way > different parameters than the original. This can lead to > miscompilations in cases like the testcase. > > This patch solves it by 1) making the inliner set the > can_change_signature flag to false for them because their signature > cannot be changed (this step is also necessary to make IPA-CP operate > on them and handle them correctly), and 2) make the splitting pass > keep all parameters if the flag is set. The second step might involve > inventing some default definitions if the parameters did not really > have any. > > I spoke about this with Honza and he claimed that the new function is > really an entirely different thing and that the parameters may > correspond only very loosely and thus the type attributes should be > cleared. I'm not sure I agree, but in any case I needed this to work
For sure some attributes may be worth to preserve/change for optimziation purposes. Now, if a function is clonable then clearing all attributes from the clone should be ok - there may be attributes that prevent cloning though (or rather, need to be preserved). Richard. > to allow me continue with promised IPA-CP polishing and so I decided > to do this because it was easier. (My own opinion is that the current > representation of parameter-describing function type attributes is > evil and will cause harm no matter hat we do.) > > A very similar patch has passed bootstrap and testsuite on > x86_64-linux, the current one is undergoing both right now. OK for > trunk if it passes? > > Thanks, > > Martin > > > > 2011-07-28 Martin Jambor <mjam...@suse.cz> > > PR middle-end/49886 > * ipa-inline-analysis.c (compute_inline_parameters): Set > can_change_signature of noes with typde attributes. > * ipa-split.c (split_function): Do not skip any arguments if > can_change_signature is set. > > * testsuite/gcc.c-torture/execute/pr49886.c: New testcase. > > Index: src/gcc/ipa-inline-analysis.c > =================================================================== > --- src.orig/gcc/ipa-inline-analysis.c > +++ src/gcc/ipa-inline-analysis.c > @@ -1658,18 +1658,24 @@ compute_inline_parameters (struct cgraph > /* Can this function be inlined at all? */ > info->inlinable = tree_inlinable_function_p (node->decl); > > - /* Inlinable functions always can change signature. */ > - if (info->inlinable) > - node->local.can_change_signature = true; > + /* Type attributes can use parameter indices to describe them. */ > + if (TYPE_ATTRIBUTES (TREE_TYPE (node->decl))) > + node->local.can_change_signature = false; > else > { > - /* Functions calling builtin_apply can not change signature. */ > - for (e = node->callees; e; e = e->next_callee) > - if (DECL_BUILT_IN (e->callee->decl) > - && DECL_BUILT_IN_CLASS (e->callee->decl) == BUILT_IN_NORMAL > - && DECL_FUNCTION_CODE (e->callee->decl) == BUILT_IN_APPLY_ARGS) > - break; > - node->local.can_change_signature = !e; > + /* Otherwise, inlinable functions always can change signature. */ > + if (info->inlinable) > + node->local.can_change_signature = true; > + else > + { > + /* Functions calling builtin_apply can not change signature. */ > + for (e = node->callees; e; e = e->next_callee) > + if (DECL_BUILT_IN (e->callee->decl) > + && DECL_BUILT_IN_CLASS (e->callee->decl) == BUILT_IN_NORMAL > + && DECL_FUNCTION_CODE (e->callee->decl) == > BUILT_IN_APPLY_ARGS) > + break; > + node->local.can_change_signature = !e; > + } > } > estimate_function_body_sizes (node, early); > > Index: src/gcc/ipa-split.c > =================================================================== > --- src.orig/gcc/ipa-split.c > +++ src/gcc/ipa-split.c > @@ -945,10 +945,10 @@ static void > split_function (struct split_point *split_point) > { > VEC (tree, heap) *args_to_pass = NULL; > - bitmap args_to_skip = BITMAP_ALLOC (NULL); > + bitmap args_to_skip; > tree parm; > int num = 0; > - struct cgraph_node *node; > + struct cgraph_node *node, *cur_node = cgraph_get_node > (current_function_decl); > basic_block return_bb = find_return_bb (); > basic_block call_bb; > gimple_stmt_iterator gsi; > @@ -968,17 +968,30 @@ split_function (struct split_point *spli > dump_split_point (dump_file, split_point); > } > > + if (cur_node->local.can_change_signature) > + args_to_skip = BITMAP_ALLOC (NULL); > + else > + args_to_skip = NULL; > + > /* Collect the parameters of new function and args_to_skip bitmap. */ > for (parm = DECL_ARGUMENTS (current_function_decl); > parm; parm = DECL_CHAIN (parm), num++) > - if (!is_gimple_reg (parm) > - || !gimple_default_def (cfun, parm) > - || !bitmap_bit_p (split_point->ssa_names_to_pass, > - SSA_NAME_VERSION (gimple_default_def (cfun, parm)))) > + if (args_to_skip > + && (!is_gimple_reg (parm) > + || !gimple_default_def (cfun, parm) > + || !bitmap_bit_p (split_point->ssa_names_to_pass, > + SSA_NAME_VERSION (gimple_default_def (cfun, > + parm))))) > bitmap_set_bit (args_to_skip, num); > else > { > arg = gimple_default_def (cfun, parm); > + if (!arg) > + { > + arg = make_ssa_name (parm, gimple_build_nop ()); > + set_default_def (parm, arg); > + } > + > if (TYPE_MAIN_VARIANT (DECL_ARG_TYPE (parm)) > != TYPE_MAIN_VARIANT (TREE_TYPE (arg))) > { > @@ -1081,9 +1094,7 @@ split_function (struct split_point *spli > > /* Now create the actual clone. */ > rebuild_cgraph_edges (); > - node = cgraph_function_versioning (cgraph_get_node (current_function_decl), > - NULL, NULL, > - args_to_skip, > + node = cgraph_function_versioning (cur_node, NULL, NULL, args_to_skip, > split_point->split_bbs, > split_point->entry_bb, "part"); > /* For usual cloning it is enough to clear builtin only when signature > @@ -1094,7 +1105,7 @@ split_function (struct split_point *spli > DECL_BUILT_IN_CLASS (node->decl) = NOT_BUILT_IN; > DECL_FUNCTION_CODE (node->decl) = (enum built_in_function) 0; > } > - cgraph_node_remove_callees (cgraph_get_node (current_function_decl)); > + cgraph_node_remove_callees (cur_node); > if (!split_part_return_p) > TREE_THIS_VOLATILE (node->decl) = 1; > if (dump_file) > Index: src/gcc/testsuite/gcc.c-torture/execute/pr49886.c > =================================================================== > --- /dev/null > +++ src/gcc/testsuite/gcc.c-torture/execute/pr49886.c > @@ -0,0 +1,100 @@ > +struct PMC { > + unsigned flags; > +}; > + > +typedef struct Pcc_cell > +{ > + struct PMC *p; > + long bla; > + long type; > +} Pcc_cell; > + > +int gi; > +int cond; > + > +extern void abort (); > +extern void never_ever(int interp, struct PMC *pmc) > + __attribute__((noinline,noclone)); > + > +void never_ever (int interp, struct PMC *pmc) > +{ > + abort (); > +} > + > +static void mark_cell(int * interp, Pcc_cell *c) > + __attribute__((__nonnull__(1))); > + > +static void > +mark_cell(int * interp, Pcc_cell *c) > +{ > + if (!cond) > + return; > + > + if (c && c->type == 4 && c->p > + && !(c->p->flags & (1<<18))) > + never_ever(gi + 1, c->p); > + if (c && c->type == 4 && c->p > + && !(c->p->flags & (1<<17))) > + never_ever(gi + 2, c->p); > + if (c && c->type == 4 && c->p > + && !(c->p->flags & (1<<16))) > + never_ever(gi + 3, c->p); > + if (c && c->type == 4 && c->p > + && !(c->p->flags & (1<<15))) > + never_ever(gi + 4, c->p); > + if (c && c->type == 4 && c->p > + && !(c->p->flags & (1<<14))) > + never_ever(gi + 5, c->p); > + if (c && c->type == 4 && c->p > + && !(c->p->flags & (1<<13))) > + never_ever(gi + 6, c->p); > + if (c && c->type == 4 && c->p > + && !(c->p->flags & (1<<12))) > + never_ever(gi + 7, c->p); > + if (c && c->type == 4 && c->p > + && !(c->p->flags & (1<<11))) > + never_ever(gi + 8, c->p); > + if (c && c->type == 4 && c->p > + && !(c->p->flags & (1<<10))) > + never_ever(gi + 9, c->p); > +} > + > +static void > +foo(int * interp, Pcc_cell *c) > +{ > + mark_cell(interp, c); > +} > + > +static struct Pcc_cell * > +__attribute__((noinline,noclone)) > +getnull(void) > +{ > + return (struct Pcc_cell *) 0; > +} > + > + > +int main() > +{ > + int i; > + > + cond = 1; > + for (i = 0; i < 100; i++) > + foo (&gi, getnull ()); > + return 0; > +} > + > + > +void > +bar_1 (int * interp, Pcc_cell *c) > +{ > + c->bla += 1; > + mark_cell(interp, c); > +} > + > +void > +bar_2 (int * interp, Pcc_cell *c) > +{ > + c->bla += 2; > + mark_cell(interp, c); > +} > + >