Re: [patch] Compare addresses in operand_equal_p correctly

[Roger Sayle <roger at eyesopen dot com>]

On Fri, 10 Nov 2006, Zdenek Dvorak wrote:
>
> 	* tree.h (enum operand_equal_flag): Add OEP_IN_ADDRESS.
> 	* fold-const.c (operand_equal_p): Do not take side effects into account
> 	when OEP_IN_ADDRESS is passed in flags.

I like the general idea, but there are a number of issues...


> !       if (!(flags & OEP_IN_ADDRESS)
> ! 	  && (TREE_SIDE_EFFECTS (arg0)
> ! 	      || TREE_SIDE_EFFECTS (arg1)))
> ! 	return 0;

It's probably better to test flags after testing for side-effects, as
this will avoid affecting the typical-case critical path.


> !       /* Types of structures must match.  */
> !       if (AGGREGATE_TYPE_P (TREE_TYPE (arg0))
> ! 	  && TREE_TYPE (arg0) != TREE_TYPE (arg1))
>   	return 0;

You'll need to explain why this is necessary and not mentioned in
either your description or the ChangeLog entry.  Checking pointer
equality for type matching feels wrong wrong.  Perhaps this needs
to be handled elsewhere in operand_equal_p, may be explicitly in
the INDIRECT_REF, ARRAY_REF, ADDR_EXPR and/or related tree codes.

> ! 	  return (OP_SAME (0)
> ! 		  && OP_SAME (1)
>   		  && OP_SAME_WITH_NULL (2)
>   		  && OP_SAME_WITH_NULL (3));

You don't need the outermost parenthesis around return expressions
in the GNU coding style.

>       case tcc_declaration:
> +       if (TREE_CODE (arg0) == FIELD_DECL)
> + 	return arg0 == arg1;
> +
> +       if ((flags & OEP_IN_ADDRESS) != 0
> + 	  && SSA_VAR_P (arg0))
> + 	return arg0 == arg1;
> +

This hunk shouldn't be needed as "arg0 == arg1" cases have already
been handled further up in operand_equal_p.  This switch statement
is only reached when arg0 != arg1 (or the expression has side-effects).


It would also be nice to have a new test-case or two to demonstrate
the types of equality that you're hoping to catch, and an example of
the aggegrate problems you're trying to avoid.

Roger
--