Ping. Re-bootstrapped and re-tested yesterday on x86_64-linux. THanks,
Martin On Fri, Jul 29, 2011 at 10:55:31PM +0200, Martin Jambor wrote: > Hi, > > On Thu, Jul 28, 2011 at 06:52:05PM +0200, Martin Jambor wrote: > > 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 > > 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.) > > > > Actually, I'd like to commit the patch below which also clears > can_change_signature for BUILT_IN_VA_START. It is not really > necessary for this fix but fixes some problems in a followup patch and > is also the correct thing to do because if we clone a function calling > it and pass non-NULL for args_to_skip, the new clone would not have a > stdarg_p type and fold_builtin_next_arg could error when dealing with > it. > > Also bootstrapped and tested on x86_64-linux. OK for trunk? > > Thanks, > > Martin > > > 2011-07-29 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,28 @@ 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) > + { > + tree cdecl = e->callee->decl; > + if (DECL_BUILT_IN (cdecl) > + && DECL_BUILT_IN_CLASS (cdecl) == BUILT_IN_NORMAL > + && (DECL_FUNCTION_CODE (cdecl) == BUILT_IN_APPLY_ARGS > + || DECL_FUNCTION_CODE (cdecl) == BUILT_IN_VA_START)) > + 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); > +} > +