On Fri, 22 May 2015, Jan Hubicka wrote:
> Hi,
> I am working on patch that makes operand_equal_p replace logic from
> ipa-icf-gimple's compare_op via a valueizer hook. Currently the patch however
> cuts number of merges on firefox to half (apparently becuase it gives up on
> some tree codes too early)
> The patch bellow merges code from ipa-icf-gimple.c that is missing in
> fold-const and is needed.
>
> Bootstrapped/regtested x86_64-linux, OK?
No, I don't like this.
> Honza
>
> * fold-const.c (operand_equal_p): Handle OBJ_TYPE_REF, CONSTRUCTOR
> and be more tolerant about FIELD_DECL.
> * tree.c (add_expr): Handle FIELD_DECL.
> (prototype_p, virtual_method_call_p, obj_type_ref_class): Constify.
> * tree.h (prototype_p, virtual_method_call_p, obj_type_ref_class):
> Constify.
> Index: fold-const.c
> ===================================================================
> --- fold-const.c (revision 223500)
> @@ -3037,6 +3058,40 @@ operand_equal_p (const_tree arg0, const_
> case DOT_PROD_EXPR:
> return OP_SAME (0) && OP_SAME (1) && OP_SAME (2);
>
> + /* OBJ_TYPE_REF really is just a transparent wrapper around expression,
> + but it holds additional type information for devirtualization that
> + needs to be matched. We may want to intoruce OEP flag if we want
> + to compare the actual value only, or if we also care about effects
> + of potentially merging the code. This flag can bypass this check
> + as well as the alias set matching in MEM_REF. */
> + case OBJ_TYPE_REF:
> + {
> + if (!operand_equal_p (OBJ_TYPE_REF_EXPR (arg0),
> + OBJ_TYPE_REF_EXPR (arg1), flags))
> + return false;
> + if (flag_devirtualize && virtual_method_call_p (arg0))
> + {
> + if (tree_to_uhwi (OBJ_TYPE_REF_TOKEN (arg0))
> + != tree_to_uhwi (OBJ_TYPE_REF_TOKEN (arg1)))
> + return false;
> + if (!types_same_for_odr (obj_type_ref_class (arg0),
> + obj_type_ref_class (arg1)))
> + return false;
devirt machinery in operand_equal_p? please not. not more places!
> + /* OBJ_TYPE_REF_OBJECT is used to track the instance of
> + object THIS pointer points to. Checking that both
> + addresses equal is safe approximation of the fact
> + that dynamic types are equal.
> + Do not care about the other flags, because this expression
> + does not attribute to actual value of OBJ_TYPE_REF */
> + if (!operand_equal_p (OBJ_TYPE_REF_OBJECT (arg0),
> + OBJ_TYPE_REF_OBJECT (arg1),
> + OEP_ADDRESS_OF))
> + return false;
> + }
> +
> + return true;
> + }
> +
> default:
> return 0;
> }
> @@ -3097,6 +3152,21 @@ operand_equal_p (const_tree arg0, const_
> }
>
> case tcc_declaration:
> + /* At LTO time the FIELD_DECL may exist in multiple copies.
> + We only care about offsets and bit offsets for operands. */
Err? Even at LTO time FIELD_DECLs should only appear once.
So - testcase?
IMHO most of this boils down to ICF being too strict about aliasing
because it inlines merged bodies instead of original ones.
> + if (TREE_CODE (arg0) == FIELD_DECL)
> + {
> + tree offset1 = DECL_FIELD_OFFSET (arg0);
> + tree offset2 = DECL_FIELD_OFFSET (arg1);
> +
> + tree bit_offset1 = DECL_FIELD_BIT_OFFSET (arg0);
> + tree bit_offset2 = DECL_FIELD_BIT_OFFSET (arg1);
> +
> + flags &= ~(OEP_CONSTANT_ADDRESS_OF|OEP_ADDRESS_OF);
> +
> + return operand_equal_p (offset1, offset2, flags)
> + && operand_equal_p (bit_offset1, bit_offset2, flags);
> + }
> /* Consider __builtin_sqrt equal to sqrt. */
> return (TREE_CODE (arg0) == FUNCTION_DECL
> && DECL_BUILT_IN (arg0) && DECL_BUILT_IN (arg1)
> @@ -3104,12 +3174,50 @@ operand_equal_p (const_tree arg0, const_
> && DECL_FUNCTION_CODE (arg0) == DECL_FUNCTION_CODE (arg1));
>
> default:
case tcc_exceptional:
> + /* In GIMPLE empty constructors are allowed in initializers of
> + vector types. */
Why this comment about GIMPLE? This is about comparing GENERIC
trees which of course can have CONSTRUCTORs of various sorts.
> + if (TREE_CODE (arg0) == CONSTRUCTOR)
> + {
> + unsigned length1 = vec_safe_length (CONSTRUCTOR_ELTS (arg0));
> + unsigned length2 = vec_safe_length (CONSTRUCTOR_ELTS (arg1));
> +
> + if (length1 != length2)
> + return false;
> +
> + flags &= ~(OEP_CONSTANT_ADDRESS_OF|OEP_ADDRESS_OF);
> +
> + for (unsigned i = 0; i < length1; i++)
> + if (!operand_equal_p (CONSTRUCTOR_ELT (arg0, i)->value,
> + CONSTRUCTOR_ELT (arg1, i)->value, flags))
You need to compare ->index as well here. I'm not sure constructor
values are sorted always so you might get very many false negatives.
> + return false;
> +
> + return true;
> + }
> return 0;
> }
>
> #undef OP_SAME
> #undef OP_SAME_WITH_NULL
> }
> Index: tree.c
> ===================================================================
> --- tree.c (revision 223500)
> +++ tree.c (working copy)
> @@ -7647,6 +7647,12 @@ add_expr (const_tree t, inchash::hash &h
> }
> return;
> }
> + case FIELD_DECL:
> + /* At LTO time the FIELD_DECL may exist in multiple copies.
> + We only care about offsets and bit offsets for operands. */
So explain that this is the reason we don't want to hash by
DECL_UID. I still think this is bogus.
> + inchash::add_expr (DECL_FIELD_OFFSET (t), hstate);
> + inchash::add_expr (DECL_FIELD_BIT_OFFSET (t), hstate);
> + return;
> case FUNCTION_DECL:
> /* When referring to a built-in FUNCTION_DECL, use the __builtin__
> form.
> Otherwise nodes that compare equal according to operand_equal_p might
> @@ -11579,7 +11585,7 @@ stdarg_p (const_tree fntype)
> /* Return true if TYPE has a prototype. */
>
> bool
> -prototype_p (tree fntype)
> +prototype_p (const_tree fntype)
This and the following look like independent (and obvious) changes.
> {
> tree t;
>
> @@ -12005,7 +12011,7 @@ lhd_gcc_personality (void)
> can't apply.) */
>
> bool
> -virtual_method_call_p (tree target)
> +virtual_method_call_p (const_tree target)
> {
> if (TREE_CODE (target) != OBJ_TYPE_REF)
> return false;
> @@ -12026,7 +12032,7 @@ virtual_method_call_p (tree target)
> /* REF is OBJ_TYPE_REF, return the class the ref corresponds to. */
>
> tree
> -obj_type_ref_class (tree ref)
> +obj_type_ref_class (const_tree ref)
> {
> gcc_checking_assert (TREE_CODE (ref) == OBJ_TYPE_REF);
> ref = TREE_TYPE (ref);
> Index: tree.h
> ===================================================================
> --- tree.h (revision 223500)
> +++ tree.h (working copy)
> @@ -4375,7 +4375,7 @@ extern int operand_equal_for_phi_arg_p (
> extern tree create_artificial_label (location_t);
> extern const char *get_name (tree);
> extern bool stdarg_p (const_tree);
> -extern bool prototype_p (tree);
> +extern bool prototype_p (const_tree);
> extern bool is_typedef_decl (tree x);
> extern bool typedef_variant_p (tree);
> extern bool auto_var_in_fn_p (const_tree, const_tree);
> @@ -4544,8 +4544,8 @@ extern location_t *block_nonartificial_l
> extern location_t tree_nonartificial_location (tree);
> extern tree block_ultimate_origin (const_tree);
> extern tree get_binfo_at_offset (tree, HOST_WIDE_INT, tree);
> -extern bool virtual_method_call_p (tree);
> -extern tree obj_type_ref_class (tree ref);
> +extern bool virtual_method_call_p (const_tree);
> +extern tree obj_type_ref_class (const_tree ref);
> extern bool types_same_for_odr (const_tree type1, const_tree type2,
> bool strict=false);
> extern bool contains_bitfld_component_ref_p (const_tree);
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham
Norton, HRB 21284 (AG Nuernberg)
