Re: [PATCH] Enable IPA-CP on functions with variable number of arguments or type attributes

[Richard Guenther <richard dot guenther at gmail dot com>]

On Fri, Sep 2, 2011 at 2:27 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> currently IPA-CP switches itself off on functions with are called with
> variable number of arguments or on those whose signature cannot be
> changed because of type attributes. ?This patch turns it on for both,
> not changing the parameters of its clones in the latter case.
>
> Whether the function's signature can be changed is determined by
> looking at node->local.can_change_signature which is set appropriately
> already since my last patch (rev 178386).
>
> This patch also replaces ipa-prop's own node_versionable flag with
> inline_summary (node)->versionable. ?Frankly, I find it very confusing
> that this flag is in the inline summary even though it is set in
> ipa-prop (except for thunks - a case we currently never care about
> anyway) and used only in ipa-cp. ?I also think this is a property of
> the node (rather than an inlining parameter) as much as
> can_change_signature is - would a patch moving it to node->local be
> acceptable?

I think so, or even node->global?

> The patch passes bootstrap and testsuite on x86_64-linux. ?It also
> successfully LTO-builds 465.tonto where it creates 647 clones (498 of
> them replacing the original function completely) without changing the
> signature of a single one. ?(No change in the run-time on this
> benchmark, though).
>
> OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
>
> Martin
>
>
>
> 2011-09-01 ?Martin Jambor ?<mjambor@suse.cz>
>
> ? ? ? ?* ipa-prop.h (ipa_node_params): Removed fields
> ? ? ? ?called_with_var_arguments and node_versionable.
> ? ? ? ?(ipa_set_called_with_variable_arg): Removed.
> ? ? ? ?(ipa_is_called_with_var_arguments): Likewise.
> ? ? ? ?* ipa-cp.c (ipa_get_lattice): Fixed index check in an assert.
> ? ? ? ?(determine_versionability): Do not check for type attributes and va
> ? ? ? ?builtins. ?Record versionability into inline summary.
> ? ? ? ?(initialize_node_lattices): Do not check
> ? ? ? ?ipa_is_called_with_var_arguments.
> ? ? ? ?(propagate_constants_accross_call): Likewise, ignore arguments we do
> ? ? ? ?not have PARM_DECLs for, set variable flag for parameters that were
> ? ? ? ?not passed a value.
> ? ? ? ?(create_specialized_node): Dump info that we cannot change signature.
> ? ? ? ?* ipa-prop.c (ipa_compute_jump_functions): Do not care about variable
> ? ? ? ?number of arguments.
> ? ? ? ?(ipa_make_edge_direct_to_target): Likewise.
> ? ? ? ?(ipa_update_after_lto_read): Likewise.
> ? ? ? ?(ipa_node_duplication_hook): Do not copy called_with_var_arguments flag.
> ? ? ? ?* tree-inline.c (copy_arguments_for_versioning): Copy PARM_DECLs if
> ? ? ? ?they were remapped.
>
> ? ? ? ?* testsuite/gcc.dg/ipa/ipcp-3.c: New test.
>
> Index: src/gcc/ipa-cp.c
> ===================================================================
> --- src.orig/gcc/ipa-cp.c
> +++ src/gcc/ipa-cp.c
> @@ -221,7 +221,7 @@ static struct ipcp_value *values_topo;
> ?static inline struct ipcp_lattice *
> ?ipa_get_lattice (struct ipa_node_params *info, int i)
> ?{
> - ?gcc_assert (i >= 0 && i <= ipa_get_param_count (info));
> + ?gcc_assert (i >= 0 && i < ipa_get_param_count (info));
> ? gcc_checking_assert (!info->ipcp_orig_node);
> ? gcc_checking_assert (info->lattices);
> ? return &(info->lattices[i]);
> @@ -360,7 +360,6 @@ print_all_lattices (FILE * f, bool dump_
> ?static void
> ?determine_versionability (struct cgraph_node *node)
> ?{
> - ?struct cgraph_edge *edge;
> ? const char *reason = NULL;
>
> ? /* There are a number of generic reasons functions cannot be versioned. ?We
> @@ -369,32 +368,15 @@ determine_versionability (struct cgraph_
> ? if (node->alias || node->thunk.thunk_p)
> ? ? reason = "alias or thunk";
> ? else if (!inline_summary (node)->versionable)
> - ? ?reason = "inliner claims it is so";
> - ?else if (TYPE_ATTRIBUTES (TREE_TYPE (node->decl)))
> - ? ?reason = "there are type attributes";
> + ? ?reason = "not a tree_versionable_function";
> ? else if (cgraph_function_body_availability (node) <= AVAIL_OVERWRITABLE)
> ? ? reason = "insufficient body availability";
> - ?else
> - ? ?/* Removing arguments doesn't work if the function takes varargs
> - ? ? ? or use __builtin_apply_args.
> - ? ? ? FIXME: handle this together with can_change_signature flag. ?*/
> - ? ?for (edge = node->callees; edge; edge = edge->next_callee)
> - ? ? ?{
> - ? ? ? tree t = edge->callee->decl;
> - ? ? ? if (DECL_BUILT_IN_CLASS (t) == BUILT_IN_NORMAL
> - ? ? ? ? ? && (DECL_FUNCTION_CODE (t) == BUILT_IN_APPLY_ARGS
> - ? ? ? ? ? ? ? || DECL_FUNCTION_CODE (t) == BUILT_IN_VA_START))
> - ? ? ? ? {
> - ? ? ? ? ? reason = "prohibitive builtins called";
> - ? ? ? ? ? break;
> - ? ? ? ? };
> - ? ? ?}
>
> ? if (reason && dump_file && !node->alias && !node->thunk.thunk_p)
> ? ? fprintf (dump_file, "Function %s/%i is not versionable, reason: %s.\n",
> ? ? ? ? ? ? cgraph_node_name (node), node->uid, reason);
>
> - ?IPA_NODE_REF (node)->node_versionable = (reason == NULL);
> + ?inline_summary (node)->versionable = (reason == NULL);
> ?}
>
> ?/* Return true if it is at all technically possible to create clones of a
> @@ -403,7 +385,7 @@ determine_versionability (struct cgraph_
> ?static bool
> ?ipcp_versionable_function_p (struct cgraph_node *node)
> ?{
> - ?return IPA_NODE_REF (node)->node_versionable;
> + ?return inline_summary (node)->versionable;
> ?}
>
> ?/* Structure holding accumulated information about callers of a node. ?*/
> @@ -610,9 +592,7 @@ initialize_node_lattices (struct cgraph_
> ? int i;
>
> ? gcc_checking_assert (cgraph_function_with_gimple_body_p (node));
> - ?if (ipa_is_called_with_var_arguments (info))
> - ? ?disable = true;
> - ?else if (!node->local.local)
> + ?if (!node->local.local)
> ? ? {
> ? ? ? /* When cloning is allowed, we can assume that externally visible
> ? ? ? ? functions are not called. ?We will compensate this by cloning
> @@ -1068,18 +1048,17 @@ propagate_constants_accross_call (struct
> ? struct cgraph_node *callee, *alias_or_thunk;
> ? struct ipa_edge_args *args;
> ? bool ret = false;
> - ?int i, count;
> + ?int i, args_count, parms_count;
>
> ? callee = cgraph_function_node (cs->callee, &availability);
> ? if (!callee->analyzed)
> ? ? return false;
> ? gcc_checking_assert (cgraph_function_with_gimple_body_p (callee));
> ? callee_info = IPA_NODE_REF (callee);
> - ?if (ipa_is_called_with_var_arguments (callee_info))
> - ? ?return false;
>
> ? args = IPA_EDGE_REF (cs);
> - ?count = ipa_get_cs_argument_count (args);
> + ?args_count = ipa_get_cs_argument_count (args);
> + ?parms_count = ipa_get_param_count (callee_info);
>
> ? /* If this call goes through a thunk we must not propagate to the first (0th)
> ? ? ?parameter. ?However, we might need to uncover a thunk from below a series
> @@ -1095,7 +1074,7 @@ propagate_constants_accross_call (struct
> ? else
> ? ? i = 0;
>
> - ?for (; i < count; i++)
> + ?for (; (i < args_count) && (i < parms_count); i++)
> ? ? {
> ? ? ? struct ipa_jump_func *jump_func = ipa_get_ith_jump_func (args, i);
> ? ? ? struct ipcp_lattice *dest_lat = ipa_get_lattice (callee_info, i);
> @@ -1105,6 +1084,9 @@ propagate_constants_accross_call (struct
> ? ? ? else
> ? ? ? ?ret |= propagate_accross_jump_function (cs, jump_func, dest_lat);
> ? ? }
> + ?for (; i < parms_count; i++)
> + ? ?ret |= set_lattice_contains_variable (ipa_get_lattice (callee_info, i));
> +
> ? return ret;
> ?}
>
> @@ -2004,7 +1986,11 @@ create_specialized_node (struct cgraph_n
> ? ? ? ?}
> ? ? }
> ? else
> - ? ?args_to_skip = NULL;
> + ? ?{
> + ? ? ?args_to_skip = NULL;
> + ? ? ?if (dump_file && (dump_flags & TDF_DETAILS))
> + ? ? ? fprintf (dump_file, " ? ? ?cannot change function signature\n");
> + ? ?}
>
> ? for (i = 0; i < count ; i++)
> ? ? {
> Index: src/gcc/ipa-prop.h
> ===================================================================
> --- src.orig/gcc/ipa-prop.h
> +++ src/gcc/ipa-prop.h
> @@ -168,11 +168,6 @@ struct ipa_node_params
> ? /* If this node is an ipa-cp clone, these are the known values that describe
> ? ? ?what it has been specialized for. ?*/
> ? VEC (tree, heap) *known_vals;
> - ?/* Whether this function is called with variable number of actual
> - ? ? arguments. ?*/
> - ?unsigned called_with_var_arguments : 1;
> - ?/* Set when it is possible to create specialized versions of this node. ?*/
> - ?unsigned node_versionable : 1;
> ? /* Whether the param uses analysis has already been performed. ?*/
> ? unsigned uses_analysis_done : 1;
> ? /* Whether the function is enqueued in ipa-cp propagation stack. ?*/
> @@ -224,22 +219,6 @@ ipa_is_param_used (struct ipa_node_param
> ? return VEC_index (ipa_param_descriptor_t, info->descriptors, i)->used;
> ?}
>
> -/* Flag this node as having callers with variable number of arguments. ?*/
> -
> -static inline void
> -ipa_set_called_with_variable_arg (struct ipa_node_params *info)
> -{
> - ?info->called_with_var_arguments = 1;
> -}
> -
> -/* Have we detected this node was called with variable number of arguments? */
> -
> -static inline bool
> -ipa_is_called_with_var_arguments (struct ipa_node_params *info)
> -{
> - ?return info->called_with_var_arguments;
> -}
> -
> ?/* ipa_edge_args stores information related to a callsite and particularly its
> ? ?arguments. ?It can be accessed by the IPA_EDGE_REF macro. ?*/
> ?typedef struct GTY(()) ipa_edge_args
> Index: src/gcc/tree-inline.c
> ===================================================================
> --- src.orig/gcc/tree-inline.c
> +++ src/gcc/tree-inline.c
> @@ -4869,6 +4869,8 @@ copy_arguments_for_versioning (tree orig
> ? ? if (!args_to_skip || !bitmap_bit_p (args_to_skip, i))
> ? ? ? {
> ? ? ? ? tree new_tree = remap_decl (arg, id);
> + ? ? ? if (TREE_CODE (new_tree) != PARM_DECL)
> + ? ? ? ? new_tree = id->copy_decl (arg, id);
> ? ? ? ? lang_hooks.dup_lang_specific_decl (new_tree);
> ? ? ? ? *parg = new_tree;
> ? ? ? ?parg = &DECL_CHAIN (new_tree);
> Index: src/gcc/testsuite/gcc.dg/ipa/ipcp-3.c
> ===================================================================
> --- /dev/null
> +++ src/gcc/testsuite/gcc.dg/ipa/ipcp-3.c
> @@ -0,0 +1,70 @@
> +/* Verify that IPA-CP can clone mark_cell without miscompiling it despite its
> + ? type_attributes. ?*/
> +/* { dg-do run } */
> +/* { dg-options "-O3 -fdump-ipa-cp" } */
> +
> +
> +struct PMC {
> + ? ?unsigned flags;
> +};
> +
> +typedef struct Pcc_cell
> +{
> + ? ?struct PMC *p;
> + ? ?long bla;
> + ? ?long type;
> +} Pcc_cell;
> +
> +int gi;
> +
> +extern void abort ();
> +extern void never_ever(int * interp, struct PMC *pmc)
> + ? ? __attribute__((noinline));
> +
> +void never_ever (int * interp, struct PMC *pmc)
> +{
> + ?abort ();
> +}
> +
> +static void mark_cell(int * interp, Pcc_cell *c)
> + ? ? ? ?__attribute__((__nonnull__(1)))
> + ? ? ? ?__attribute__((noinline));
> +
> +static void
> +mark_cell(int * interp, Pcc_cell *c)
> +{
> + ?if (c && c->type == 4 && c->p
> + ? ? ?&& !(c->p->flags & (1<<18)))
> + ? ?never_ever(interp, c->p);
> +}
> +
> +static void foo(int * interp, Pcc_cell *c)
> + ?__attribute__((noinline));
> +
> +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;
> +
> + ?for (i = 0; i < 100; i++)
> + ? ?foo (&gi, getnull ());
> + ?return 0;
> +}
> +
> +
> +/* { dg-final { scan-ipa-dump "Creating a specialized node of mark_cell" "cp" } } */
> +/* { dg-final { cleanup-ipa-dump "cp" } } */
> +
> Index: src/gcc/ipa-prop.c
> ===================================================================
> --- src.orig/gcc/ipa-prop.c
> +++ src/gcc/ipa-prop.c
> @@ -1032,19 +1032,13 @@ ipa_compute_jump_functions (struct cgrap
>
> ? for (cs = node->callees; cs; cs = cs->next_callee)
> ? ? {
> - ? ? ?struct cgraph_node *callee = cgraph_function_or_thunk_node (cs->callee, NULL);
> + ? ? ?struct cgraph_node *callee = cgraph_function_or_thunk_node (cs->callee,
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? NULL);
> ? ? ? /* We do not need to bother analyzing calls to unknown
> ? ? ? ? functions unless they may become known during lto/whopr. ?*/
> - ? ? ?if (!cs->callee->analyzed && !flag_lto)
> + ? ? ?if (!callee->analyzed && !flag_lto)
> ? ? ? ?continue;
> ? ? ? ipa_count_arguments (cs);
> - ? ? ?/* If the descriptor of the callee is not initialized yet, we have to do
> - ? ? ? ?it now. */
> - ? ? ?if (callee->analyzed)
> - ? ? ? ipa_initialize_node_params (callee);
> - ? ? ?if (ipa_get_cs_argument_count (IPA_EDGE_REF (cs))
> - ? ? ? ? != ipa_get_param_count (IPA_NODE_REF (callee)))
> - ? ? ? ipa_set_called_with_variable_arg (IPA_NODE_REF (callee));
> ? ? ? ipa_compute_jump_functions_for_edge (parms_info, cs);
> ? ? }
>
> @@ -1649,10 +1643,6 @@ ipa_make_edge_direct_to_target (struct c
> ? ? }
> ? callee = cgraph_function_or_thunk_node (callee, NULL);
>
> - ?if (ipa_get_cs_argument_count (IPA_EDGE_REF (ie))
> - ? ? ?!= ipa_get_param_count (IPA_NODE_REF (callee)))
> - ? ?ipa_set_called_with_variable_arg (IPA_NODE_REF (callee));
> -
> ? return ie;
> ?}
>
> @@ -1964,7 +1954,6 @@ ipa_node_duplication_hook (struct cgraph
> ? new_info->lattices = NULL;
> ? new_info->ipcp_orig_node = old_info->ipcp_orig_node;
>
> - ?new_info->called_with_var_arguments = old_info->called_with_var_arguments;
> ? new_info->uses_analysis_done = old_info->uses_analysis_done;
> ? new_info->node_enqueued = old_info->node_enqueued;
> ?}
> @@ -2949,7 +2938,6 @@ void
> ?ipa_update_after_lto_read (void)
> ?{
> ? struct cgraph_node *node;
> - ?struct cgraph_edge *cs;
>
> ? ipa_check_create_node_params ();
> ? ipa_check_create_edge_args ();
> @@ -2957,17 +2945,4 @@ ipa_update_after_lto_read (void)
> ? for (node = cgraph_nodes; node; node = node->next)
> ? ? if (node->analyzed)
> ? ? ? ipa_initialize_node_params (node);
> -
> - ?for (node = cgraph_nodes; node; node = node->next)
> - ? ?if (node->analyzed)
> - ? ? ?for (cs = node->callees; cs; cs = cs->next_callee)
> - ? ? ? {
> - ? ? ? ? struct cgraph_node *callee;
> -
> - ? ? ? ? callee = cgraph_function_or_thunk_node (cs->callee, NULL);
> - ? ? ? ? if (ipa_get_cs_argument_count (IPA_EDGE_REF (cs))
> - ? ? ? ? ? ? != ipa_get_param_count (IPA_NODE_REF (callee)))
> - ? ? ? ? ? ipa_set_called_with_variable_arg (IPA_NODE_REF (callee));
> - ? ? ? }
> ?}
> -
>