Re: Add few cases to operand_equal_p

[Richard Biener <rguenther at suse dot de>]

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 <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg)