On Thu, 12 Nov 2020, Jan Hubicka wrote:
> Hi,
> with ipa-icf we often run into problem that operand_equal_p does not
> match ADDR_EXPR that take address of fields from two different instances
> of same class (at ideantical offsets). Similar problem can also happen
> for record types with LTO if they did not get tree merged.
> This patch makes fold-const to compare offsets rather then pinter
> equality of FIELD_DECLs. This is done in OEP_ADDRESS_OF mode only sinc
> it is not TBAA safe and the TBAA side should be correctly solved by my
> ICF patch.
>
> Bootstrapped/regtested x86_64-linux, OK?
>
> Honza
>
> * fold-const.c (operand_compare::operand_equal_p): When comparing
> addresses
> look info field offsets for COMPONENT_REFs.
> (operand_compare::hash_operand): Likewise.
> diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> index c47557daeba..a4e8cccb1b7 100644
> --- a/gcc/fold-const.c
> +++ b/gcc/fold-const.c
> @@ -3312,9 +3312,41 @@ operand_compare::operand_equal_p (const_tree arg0,
> const_tree arg1,
> case COMPONENT_REF:
> /* Handle operand 2 the same as for ARRAY_REF. Operand 0
> may be NULL when we're called to compare MEM_EXPRs. */
> - if (!OP_SAME_WITH_NULL (0)
> - || !OP_SAME (1))
> + if (!OP_SAME_WITH_NULL (0))
> return false;
> + /* Most of time we only need to compare FIELD_DECLs for equality.
> + However when determining address look into actual offsets.
> + These may match for unions and unshared record types. */
> + if (!OP_SAME (1))
> + {
> + if (flags & OEP_ADDRESS_OF)
> + {
actually if OP2 is not NULL for both you can just compare that (and that's
more correct then).
> + tree field0 = TREE_OPERAND (arg0, 1);
> + tree field1 = TREE_OPERAND (arg1, 1);
> + tree type0 = DECL_CONTEXT (field0);
> + tree type1 = DECL_CONTEXT (field1);
> +
> + if (TREE_CODE (type0) == RECORD_TYPE
> + && DECL_BIT_FIELD_REPRESENTATIVE (field0))
> + field0 = DECL_BIT_FIELD_REPRESENTATIVE (field0);
> + if (TREE_CODE (type1) == RECORD_TYPE
> + && DECL_BIT_FIELD_REPRESENTATIVE (field1))
> + field1 = DECL_BIT_FIELD_REPRESENTATIVE (field1);
Why does the representative matter? For a 32bit bitfield if you'd
have two addresses at 8bit boundary but different you'd make them
equal this way. Soo ...
> + /* Assume that different FIELD_DECLs never overlap within a
> + RECORD_TYPE. */
> + if (type0 == type1 && TREE_CODE (type0) == RECORD_TYPE)
> + return false;
this isn't really about "overlap", OEP_ADDRESS_OF is just about
the address (not it's extent).
> + if (!operand_equal_p (DECL_FIELD_OFFSET (field0),
> + DECL_FIELD_OFFSET (field1),
> + flags & ~OEP_ADDRESS_OF)
> + || !operand_equal_p (DECL_FIELD_BIT_OFFSET (field0),
> + DECL_FIELD_BIT_OFFSET (field1),
> + flags & ~OEP_ADDRESS_OF))
> + return false;
So this should suffice (on the original fields).
> + }
> + else
> + return false;
> + }
> flags &= ~OEP_ADDRESS_OF;
> return OP_SAME_WITH_NULL (2);
>
> @@ -3787,9 +3819,28 @@ operand_compare::hash_operand (const_tree t,
> inchash::hash &hstate,
> sflags = flags;
> break;
>
> + case COMPONENT_REF:
> + if (flags & OEP_ADDRESS_OF)
> + {
> + tree field = TREE_OPERAND (t, 1);
> + tree type = DECL_CONTEXT (field);
> +
> + if (TREE_CODE (type) == RECORD_TYPE
> + && DECL_BIT_FIELD_REPRESENTATIVE (field))
> + field = DECL_BIT_FIELD_REPRESENTATIVE (field);
see above.
> + hash_operand (TREE_OPERAND (t, 0), hstate, flags);
> + hash_operand (DECL_FIELD_OFFSET (field),
> + hstate, flags & ~OEP_ADDRESS_OF);
> + hash_operand (DECL_FIELD_BIT_OFFSET (field),
> + hstate, flags & ~OEP_ADDRESS_OF);
> + hash_operand (TREE_OPERAND (t, 2), hstate,
> + flags & ~OEP_ADDRESS_OF);
otherwise this looks ok.
> + return;
> + }
> + break;
> case ARRAY_REF:
> case ARRAY_RANGE_REF:
> - case COMPONENT_REF:
> case BIT_FIELD_REF:
> sflags &= ~OEP_ADDRESS_OF;
> break;
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend
