On Thu, 6 Feb 2014, Jan Hubicka wrote:
> Hi,
> at WPA we currently read trees accessed by jump functions and then copy them
> to remove location that is already known to be UNKNOWN and then keep copying
> them for every inline clone introduced (and there are many for firefox)
>
> This patch makes us to copy only when expression really has an location in it.
>
> Bootstrapped/regtested x86_64-linux, OK?
Hmm, I think you either can use just
if (EXPR_P (expr))
walk_tree (&expr, prune_expr_location, NULL, NULL);
or you miss unsharing and create invalid shared trees when
the expr does not contain locations.
I fear it's the latter, given how ipa_set_jf_* is used.
So the patch looks wrong.
Richard.
> Honza
>
> * gimplify.c (expr_with_location_p): New function.
> (expr_without_location): Likewise.
> * gimplify.h (expr_without_location): Declare.
> * ipa-prop.c (ipa_set_jf_constant, ipa_set_jf_arith_pass_through,
> determine_known_aggregate_parts): Do not unshare.
> Index: gimplify.c
> ===================================================================
> --- gimplify.c (revision 207514)
> +++ gimplify.c (working copy)
> @@ -901,6 +901,32 @@ unshare_expr_without_location (tree expr
> walk_tree (&expr, prune_expr_location, NULL, NULL);
> return expr;
> }
> +
> +/* Worker for expr_without_location. */
> +
> +static tree
> +expr_with_location_p (tree *tp, int *walk_subtrees, void *)
> +{
> + if (EXPR_P (*tp))
> + {
> + if (EXPR_LOCATION (*tp) != UNKNOWN_LOCATION)
> + return *tp;
> + }
> + else
> + *walk_subtrees = 0;
> + return NULL_TREE;
> +}
> +
> +/* Return EXPR if it has no location, otherwise make its copy
> + without location. */
> +
> +tree
> +expr_without_location (tree expr)
> +{
> + if (walk_tree (&expr, expr_with_location_p, NULL, NULL))
> + return (unshare_expr_without_location (expr));
> + return expr;
> +}
>
> /* WRAPPER is a code such as BIND_EXPR or CLEANUP_POINT_EXPR which can both
> contain statements and have a value. Assign its value to a temporary
> Index: gimplify.h
> ===================================================================
> --- gimplify.h (revision 207514)
> +++ gimplify.h (working copy)
> @@ -62,6 +62,7 @@ extern void declare_vars (tree, gimple,
> extern void gimple_add_tmp_var (tree);
> extern tree unshare_expr (tree);
> extern tree unshare_expr_without_location (tree);
> +extern tree expr_without_location (tree);
> extern tree voidify_wrapper_expr (tree, tree);
> extern tree build_and_jump (tree *);
> extern enum gimplify_status gimplify_self_mod_expr (tree *, gimple_seq *,
> Index: ipa-prop.c
> ===================================================================
> --- ipa-prop.c (revision 207514)
> +++ ipa-prop.c (working copy)
> @@ -419,11 +419,8 @@ static void
> ipa_set_jf_constant (struct ipa_jump_func *jfunc, tree constant,
> struct cgraph_edge *cs)
> {
> - constant = unshare_expr (constant);
> - if (constant && EXPR_P (constant))
> - SET_EXPR_LOCATION (constant, UNKNOWN_LOCATION);
> jfunc->type = IPA_JF_CONST;
> - jfunc->value.constant.value = unshare_expr_without_location (constant);
> + jfunc->value.constant.value = expr_without_location (constant);
>
> if (TREE_CODE (constant) == ADDR_EXPR
> && TREE_CODE (TREE_OPERAND (constant, 0)) == FUNCTION_DECL)
> @@ -463,7 +460,7 @@ ipa_set_jf_arith_pass_through (struct ip
> tree operand, enum tree_code operation)
> {
> jfunc->type = IPA_JF_PASS_THROUGH;
> - jfunc->value.pass_through.operand = unshare_expr_without_location
> (operand);
> + jfunc->value.pass_through.operand = expr_without_location (operand);
> jfunc->value.pass_through.formal_id = formal_id;
> jfunc->value.pass_through.operation = operation;
> jfunc->value.pass_through.agg_preserved = false;
> @@ -1538,7 +1535,7 @@ determine_known_aggregate_parts (gimple
> struct ipa_agg_jf_item item;
> item.offset = list->offset - arg_offset;
> gcc_assert ((item.offset % BITS_PER_UNIT) == 0);
> - item.value = unshare_expr_without_location (list->constant);
> + item.value = expr_without_location (list->constant);
> jfunc->agg.items->quick_push (item);
> }
> list = list->next;
>
>
--
Richard Biener <rguent...@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer
