Compare field offsets in fold_const when checking addresses

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.

On Thu, Nov 12, 2020 at 10:36:28AM +0100, Jan Hubicka wrote:
> 	* 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)
> +		{
> +		  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);
> +		  /* Assume that different FIELD_DECLs never overlap within a
> +		     RECORD_TYPE.  */
> +		  if (type0 == type1 && TREE_CODE (type0) == RECORD_TYPE)
> +		    return false;
> +		  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))

If it is an address, why do you need to handle
DECL_BIT_FIELD_REPRESENTATIVE?  Taking address of a bit-field is not allowed.
Perhaps just return false if the fields are bit-fields (or assert they
aren't), and just compare DECL_FIELD*OFFSET of the fields themselves?

	Jakub

> > +		  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))
> 
> If it is an address, why do you need to handle
> DECL_BIT_FIELD_REPRESENTATIVE?  Taking address of a bit-field is not allowed.
> Perhaps just return false if the fields are bit-fields (or assert they
> aren't), and just compare DECL_FIELD*OFFSET of the fields themselves?

I took the code from nonoverlapping_component_refs_p_1, however in
compare_ao_refs i call compare_operands on OEP_ADDRESS for memory
operands, so it would be useful there.  I think it makes sense in that
context - in order to match memory acesses for equivalence we want to
first compare that they access same memory location...

Honza
> 
> 	Jakub
>

On Thu, Nov 12, 2020 at 10:49:40AM +0100, Jan Hubicka wrote:
> > > +		  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))
> > 
> > If it is an address, why do you need to handle
> > DECL_BIT_FIELD_REPRESENTATIVE?  Taking address of a bit-field is not allowed.
> > Perhaps just return false if the fields are bit-fields (or assert they
> > aren't), and just compare DECL_FIELD*OFFSET of the fields themselves?
> 
> I took the code from nonoverlapping_component_refs_p_1, however in
> compare_ao_refs i call compare_operands on OEP_ADDRESS for memory
> operands, so it would be useful there.  I think it makes sense in that
> context - in order to match memory acesses for equivalence we want to
> first compare that they access same memory location...

If OEP_ADDRESS is used also on non-addressable stuff, just to compare
that two COMPONENT_REFs access the same memory, then just comparing
DECL_BIT_FIELD_REPRESENTATIVE is not sufficient, you could have:
struct S { int c; int a : 7, b : 1; };
struct T { int c; int a : 7, b : 1; };
and compare s->a vs. t->b with OEP_ADDRESS and the offsets of their
DECL_BIT_FIELD_REPRESENATIVE is the same, yet we don't want to say
the two bit-fields are the same.

	Jakub

> On Thu, Nov 12, 2020 at 10:49:40AM +0100, Jan Hubicka wrote:
> > > > +		  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))
> > > 
> > > If it is an address, why do you need to handle
> > > DECL_BIT_FIELD_REPRESENTATIVE?  Taking address of a bit-field is not allowed.
> > > Perhaps just return false if the fields are bit-fields (or assert they
> > > aren't), and just compare DECL_FIELD*OFFSET of the fields themselves?
> > 
> > I took the code from nonoverlapping_component_refs_p_1, however in
> > compare_ao_refs i call compare_operands on OEP_ADDRESS for memory
> > operands, so it would be useful there.  I think it makes sense in that
> > context - in order to match memory acesses for equivalence we want to
> > first compare that they access same memory location...
> 
> If OEP_ADDRESS is used also on non-addressable stuff, just to compare
> that two COMPONENT_REFs access the same memory, then just comparing
> DECL_BIT_FIELD_REPRESENTATIVE is not sufficient, you could have:
> struct S { int c; int a : 7, b : 1; };
> struct T { int c; int a : 7, b : 1; };
> and compare s->a vs. t->b with OEP_ADDRESS and the offsets of their
> DECL_BIT_FIELD_REPRESENATIVE is the same, yet we don't want to say
> the two bit-fields are the same.

You are right, I was just thinking of that.  I suppose it indeed makes
more sense to assert that there are no bitfields here and in the AO
comparsion take care of stripping the last bitfield reference and
handling it specially?

Honza
> 
> 	Jakub
>

On Thu, Nov 12, 2020 at 11:29:21AM +0100, Jan Hubicka wrote:
> > If OEP_ADDRESS is used also on non-addressable stuff, just to compare
> > that two COMPONENT_REFs access the same memory, then just comparing
> > DECL_BIT_FIELD_REPRESENTATIVE is not sufficient, you could have:
> > struct S { int c; int a : 7, b : 1; };
> > struct T { int c; int a : 7, b : 1; };
> > and compare s->a vs. t->b with OEP_ADDRESS and the offsets of their
> > DECL_BIT_FIELD_REPRESENATIVE is the same, yet we don't want to say
> > the two bit-fields are the same.
> 
> You are right, I was just thinking of that.  I suppose it indeed makes
> more sense to assert that there are no bitfields here and in the AO
> comparsion take care of stripping the last bitfield reference and
> handling it specially?

Or just compare DECL_FIELD_OFFSET and DECL_FIELD_BIT_OFFSET of the fields
rather than their DECL_BIT_FIELD_REPRESENTATIVE?

	Jakub

> On Thu, Nov 12, 2020 at 11:29:21AM +0100, Jan Hubicka wrote:
> > > If OEP_ADDRESS is used also on non-addressable stuff, just to compare
> > > that two COMPONENT_REFs access the same memory, then just comparing
> > > DECL_BIT_FIELD_REPRESENTATIVE is not sufficient, you could have:
> > > struct S { int c; int a : 7, b : 1; };
> > > struct T { int c; int a : 7, b : 1; };
> > > and compare s->a vs. t->b with OEP_ADDRESS and the offsets of their
> > > DECL_BIT_FIELD_REPRESENATIVE is the same, yet we don't want to say
> > > the two bit-fields are the same.
> > 
> > You are right, I was just thinking of that.  I suppose it indeed makes
> > more sense to assert that there are no bitfields here and in the AO
> > comparsion take care of stripping the last bitfield reference and
> > handling it specially?
> 
> Or just compare DECL_FIELD_OFFSET and DECL_FIELD_BIT_OFFSET of the fields
> rather than their DECL_BIT_FIELD_REPRESENTATIVE?

I think I will need to compare bitfields specially at the ao_ref_compare
side anyway to distinguish

 struct S { int c; int a : 5, b : 1; };
 struct T { int c; int a : 5, b : 3; };

s->b and t->b. Those have same base address (bitwise) but still we do
not want to consider them equal.

So handling this on operand_equal_p is probably not that useful and
perhaps extra sanity check would be.  I will fire boostrap to see if
there is any other place that trips bitfields with OEP_ADDRESS_OF.

Thanks,
Honza

On Thu, Nov 12, 2020 at 11:39:07AM +0100, Jan Hubicka wrote:
> > On Thu, Nov 12, 2020 at 11:29:21AM +0100, Jan Hubicka wrote:
> > > > If OEP_ADDRESS is used also on non-addressable stuff, just to compare
> > > > that two COMPONENT_REFs access the same memory, then just comparing
> > > > DECL_BIT_FIELD_REPRESENTATIVE is not sufficient, you could have:
> > > > struct S { int c; int a : 7, b : 1; };
> > > > struct T { int c; int a : 7, b : 1; };
> > > > and compare s->a vs. t->b with OEP_ADDRESS and the offsets of their
> > > > DECL_BIT_FIELD_REPRESENATIVE is the same, yet we don't want to say
> > > > the two bit-fields are the same.
> > > 
> > > You are right, I was just thinking of that.  I suppose it indeed makes
> > > more sense to assert that there are no bitfields here and in the AO
> > > comparsion take care of stripping the last bitfield reference and
> > > handling it specially?
> > 
> > Or just compare DECL_FIELD_OFFSET and DECL_FIELD_BIT_OFFSET of the fields
> > rather than their DECL_BIT_FIELD_REPRESENTATIVE?
> 
> I think I will need to compare bitfields specially at the ao_ref_compare
> side anyway to distinguish
> 
>  struct S { int c; int a : 5, b : 1; };
>  struct T { int c; int a : 5, b : 3; };
> 
> s->b and t->b. Those have same base address (bitwise) but still we do
> not want to consider them equal.

How is that different from:
struct S { long long d; int e; };
struct T { long long d; long long e; };
s->e vs. t->e ?
One thing is comparison of the address (as it is comparing
DECL_FIELD_BIT_OFFSET too, it is essentially bit-address), and another thing
(unlrelated to OEP_ADDRESS comparisons) is if you need to ensure the access
has the same size, in that case you just compare the bit size of the access...

	Jakub

> On Thu, Nov 12, 2020 at 11:39:07AM +0100, Jan Hubicka wrote:
> > > On Thu, Nov 12, 2020 at 11:29:21AM +0100, Jan Hubicka wrote:
> > > > > If OEP_ADDRESS is used also on non-addressable stuff, just to compare
> > > > > that two COMPONENT_REFs access the same memory, then just comparing
> > > > > DECL_BIT_FIELD_REPRESENTATIVE is not sufficient, you could have:
> > > > > struct S { int c; int a : 7, b : 1; };
> > > > > struct T { int c; int a : 7, b : 1; };
> > > > > and compare s->a vs. t->b with OEP_ADDRESS and the offsets of their
> > > > > DECL_BIT_FIELD_REPRESENATIVE is the same, yet we don't want to say
> > > > > the two bit-fields are the same.
> > > > 
> > > > You are right, I was just thinking of that.  I suppose it indeed makes
> > > > more sense to assert that there are no bitfields here and in the AO
> > > > comparsion take care of stripping the last bitfield reference and
> > > > handling it specially?
> > > 
> > > Or just compare DECL_FIELD_OFFSET and DECL_FIELD_BIT_OFFSET of the fields
> > > rather than their DECL_BIT_FIELD_REPRESENTATIVE?
> > 
> > I think I will need to compare bitfields specially at the ao_ref_compare
> > side anyway to distinguish
> > 
> >  struct S { int c; int a : 5, b : 1; };
> >  struct T { int c; int a : 5, b : 3; };
> > 
> > s->b and t->b. Those have same base address (bitwise) but still we do
> > not want to consider them equal.
> 
> How is that different from:
> struct S { long long d; int e; };
> struct T { long long d; long long e; };
> s->e vs. t->e ?
> One thing is comparison of the address (as it is comparing
> DECL_FIELD_BIT_OFFSET too, it is essentially bit-address), and another thing
> (unlrelated to OEP_ADDRESS comparisons) is if you need to ensure the access
> has the same size, in that case you just compare the bit size of the access...

I believe for that I only need to compare TYPE_SIZE of the access.
Bitfields are special since their types are wider and they do extension.

Honza
> 
> 	Jakub
>

> > How is that different from:
> > struct S { long long d; int e; };
> > struct T { long long d; long long e; };
> > s->e vs. t->e ?
> > One thing is comparison of the address (as it is comparing
> > DECL_FIELD_BIT_OFFSET too, it is essentially bit-address), and another thing
> > (unlrelated to OEP_ADDRESS comparisons) is if you need to ensure the access
> > has the same size, in that case you just compare the bit size of the access...
> 
> I believe for that I only need to compare TYPE_SIZE of the access.
> Bitfields are special since their types are wider and they do extension.

So the extra sanity check fires in

0xe3d093 operand_compare::operand_equal_p(tree_node const*, tree_node const*, unsigned int)
        ../../gcc/fold-const.c:3329
0xe4066d operand_equal_p(tree_node const*, tree_node const*, unsigned int)
        ../../gcc/fold-const.c:3949
0x167fad5 refs_hasher::equal(ref_to_bb const*, ref_to_bb const*)
        ../../gcc/tree-ssa-phiopt.c:2152
0x1680555 hash_table<refs_hasher, false, xcallocator>::find_slot_with_hash(ref_to_bb* const&, unsigned int, insert_option)
        ../../gcc/hash-table.h:981
0x16802d6 hash_table<refs_hasher, false, xcallocator>::find_slot(ref_to_bb* const&, insert_option)
        ../../gcc/hash-table.h:435
0x167b4f2 nontrapping_dom_walker::add_or_mark_expr(basic_block_def*, tree_node*, bool)
        ../../gcc/tree-ssa-phiopt.c:2261
0x167b380 nontrapping_dom_walker::before_dom_children(basic_block_def*)
        ../../gcc/tree-ssa-phiopt.c:2211
0x23705e1 dom_walker::walk(basic_block_def*)
        ../../gcc/domwalk.c:309
0x167b637 get_non_trapping
        ../../gcc/tree-ssa-phiopt.c:2308
0x167410e tree_ssa_phiopt_worker
        ../../gcc/tree-ssa-phiopt.c:177
0x1673f65 tree_ssa_cs_elim
        ../../gcc/tree-ssa-phiopt.c:127
0x167d84c execute
        ../../gcc/tree-ssa-phiopt.c:3192

This makes sort of sense: refs hasher is trying to prove that two
accesses go to exactly same location to eliminate NULL pointer checks.
If my ao_ref_compare gets approved (that is I guess not at all clear at
this point) I suppose we could use it here.  We want only accesses to
have same address&size, we do not care about TBAA properties here.

It is not obvious to me how this optimization is safe without
-fnon-call-exceptions though.

I will test the patch without assert for now.

Honza

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;
>

> > 	* 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).

We discussed this with Jakub, so I have already tested version dropping
both of these, but indeed I should check for OPERAND2.  Will do that
now.

Thanks!
Honza
> 
> > +		  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 <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
> Germany; GF: Felix Imend

Hi,
this is updated patch I am re-testing and plan to commit if it suceeds.

	* fold-const.c (operand_compare::operand_equal_p): Compare
	offsets of fields in component_refs when comparing addresses.
	(operand_compare::hash_operand): Likewise.
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index c47557daeba..273ee25ceda 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -3312,11 +3312,36 @@ 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;
-	  flags &= ~OEP_ADDRESS_OF;
-	  return OP_SAME_WITH_NULL (2);
+	  /* 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)
+		{
+		  if (TREE_OPERAND (arg0, 2)
+		      || TREE_OPERAND (arg1, 2))
+		    {
+		      flags &= ~OEP_ADDRESS_OF;
+		      return OP_SAME_WITH_NULL (2);
+		    }
+		  tree field0 = TREE_OPERAND (arg0, 1);
+		  tree field1 = TREE_OPERAND (arg1, 1);
+
+		  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;
+		}
+	      else
+		return false;
+	    }
+	  return true;
 
 	case BIT_FIELD_REF:
 	  if (!OP_SAME (0))
@@ -3787,9 +3812,26 @@ operand_compare::hash_operand (const_tree t, inchash::hash &hstate,
 	      sflags = flags;
 	      break;
 
+	    case COMPONENT_REF:
+	      if (sflags & OEP_ADDRESS_OF)
+		{
+		  hash_operand (TREE_OPERAND (t, 0), hstate, flags);
+		  if (TREE_OPERAND (t, 2))
+		    hash_operand (TREE_OPERAND (t, 2), hstate,
+				  flags & ~OEP_ADDRESS_OF);
+		  else
+		    {
+		      tree field = TREE_OPERAND (t, 1);
+		      hash_operand (DECL_FIELD_OFFSET (field),
+				    hstate, flags & ~OEP_ADDRESS_OF);
+		      hash_operand (DECL_FIELD_BIT_OFFSET (field),
+				    hstate, flags & ~OEP_ADDRESS_OF);
+		    }
+		  return;
+		}
+	      break;
 	    case ARRAY_REF:
 	    case ARRAY_RANGE_REF:
-	    case COMPONENT_REF:
 	    case BIT_FIELD_REF:
 	      sflags &= ~OEP_ADDRESS_OF;
 	      break;

On Thu, 12 Nov 2020, Jan Hubicka wrote:

> Hi,
> this is updated patch I am re-testing and plan to commit if it suceeds.
> 
> 	* fold-const.c (operand_compare::operand_equal_p): Compare
> 	offsets of fields in component_refs when comparing addresses.
> 	(operand_compare::hash_operand): Likewise.
> diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> index c47557daeba..273ee25ceda 100644
> --- a/gcc/fold-const.c
> +++ b/gcc/fold-const.c
> @@ -3312,11 +3312,36 @@ 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;
> -	  flags &= ~OEP_ADDRESS_OF;
> -	  return OP_SAME_WITH_NULL (2);
> +	  /* 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.  */

looks like you can simplify by doing

          flags &= ~OEP_ADDRESS_OF;

here.  Neither the FIELD_DECL compare nor the offsets need it

> +	  if (!OP_SAME (1))
> +	    {
> +	      if (flags & OEP_ADDRESS_OF)
> +		{
> +		  if (TREE_OPERAND (arg0, 2)
> +		      || TREE_OPERAND (arg1, 2))
> +		    {
> +		      flags &= ~OEP_ADDRESS_OF;
> +		      return OP_SAME_WITH_NULL (2);
> +		    }
> +		  tree field0 = TREE_OPERAND (arg0, 1);
> +		  tree field1 = TREE_OPERAND (arg1, 1);
> +
> +		  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;
> +		}
> +	      else
> +		return false;
> +	    }

You elided

          flags &= ~OEP_ADDRESS_OF;
-         return OP_SAME_WITH_NULL (2);

that was here when OP_SAME (1), please re-instantiate.

> +	  return true;
>  
>  	case BIT_FIELD_REF:
>  	  if (!OP_SAME (0))
> @@ -3787,9 +3812,26 @@ operand_compare::hash_operand (const_tree t, inchash::hash &hstate,
>  	      sflags = flags;
>  	      break;
>  
> +	    case COMPONENT_REF:
> +	      if (sflags & OEP_ADDRESS_OF)
> +		{
> +		  hash_operand (TREE_OPERAND (t, 0), hstate, flags);
> +		  if (TREE_OPERAND (t, 2))
> +		    hash_operand (TREE_OPERAND (t, 2), hstate,
> +				  flags & ~OEP_ADDRESS_OF);
> +		  else
> +		    {
> +		      tree field = TREE_OPERAND (t, 1);
> +		      hash_operand (DECL_FIELD_OFFSET (field),
> +				    hstate, flags & ~OEP_ADDRESS_OF);
> +		      hash_operand (DECL_FIELD_BIT_OFFSET (field),
> +				    hstate, flags & ~OEP_ADDRESS_OF);
> +		    }
> +		  return;
> +		}
> +	      break;
>  	    case ARRAY_REF:
>  	    case ARRAY_RANGE_REF:
> -	    case COMPONENT_REF:
>  	    case BIT_FIELD_REF:
>  	      sflags &= ~OEP_ADDRESS_OF;
>  	      break;
>

> On Thu, 12 Nov 2020, Jan Hubicka wrote:
> 
> > Hi,
> > this is updated patch I am re-testing and plan to commit if it suceeds.
> > 
> > 	* fold-const.c (operand_compare::operand_equal_p): Compare
> > 	offsets of fields in component_refs when comparing addresses.
> > 	(operand_compare::hash_operand): Likewise.
> > diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> > index c47557daeba..273ee25ceda 100644
> > --- a/gcc/fold-const.c
> > +++ b/gcc/fold-const.c
> > @@ -3312,11 +3312,36 @@ 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;
> > -	  flags &= ~OEP_ADDRESS_OF;
> > -	  return OP_SAME_WITH_NULL (2);
> > +	  /* 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.  */
> 
> looks like you can simplify by doing
> 
>           flags &= ~OEP_ADDRESS_OF;
> 
> here.  Neither the FIELD_DECL compare nor the offsets need it

Yep
> 
> You elided
> 
>           flags &= ~OEP_ADDRESS_OF;
> -         return OP_SAME_WITH_NULL (2);
> 
> that was here when OP_SAME (1), please re-instantiate.
Sorry for that, that was not very careful.
Here is updated patch I re-tested x86_64-linux.

diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index c47557daeba..ddf18f27cb7 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -3312,10 +3312,32 @@ 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.  */
 	  flags &= ~OEP_ADDRESS_OF;
+	  if (!OP_SAME (1))
+	    {
+	      if (flags & OEP_ADDRESS_OF)
+		{
+		  if (TREE_OPERAND (arg0, 2)
+		      || TREE_OPERAND (arg1, 2))
+		    return OP_SAME_WITH_NULL (2);
+		  tree field0 = TREE_OPERAND (arg0, 1);
+		  tree field1 = TREE_OPERAND (arg1, 1);
+
+		  if (!operand_equal_p (DECL_FIELD_OFFSET (field0),
+					DECL_FIELD_OFFSET (field1), flags)
+		      || !operand_equal_p (DECL_FIELD_BIT_OFFSET (field0),
+					   DECL_FIELD_BIT_OFFSET (field1),
+					   flags))
+		    return false;
+		}
+	      else
+		return false;
+	    }
 	  return OP_SAME_WITH_NULL (2);
 
 	case BIT_FIELD_REF:
@@ -3787,9 +3809,26 @@ operand_compare::hash_operand (const_tree t, inchash::hash &hstate,
 	      sflags = flags;
 	      break;
 
+	    case COMPONENT_REF:
+	      if (sflags & OEP_ADDRESS_OF)
+		{
+		  hash_operand (TREE_OPERAND (t, 0), hstate, flags);
+		  if (TREE_OPERAND (t, 2))
+		    hash_operand (TREE_OPERAND (t, 2), hstate,
+				  flags & ~OEP_ADDRESS_OF);
+		  else
+		    {
+		      tree field = TREE_OPERAND (t, 1);
+		      hash_operand (DECL_FIELD_OFFSET (field),
+				    hstate, flags & ~OEP_ADDRESS_OF);
+		      hash_operand (DECL_FIELD_BIT_OFFSET (field),
+				    hstate, flags & ~OEP_ADDRESS_OF);
+		    }
+		  return;
+		}
+	      break;
 	    case ARRAY_REF:
 	    case ARRAY_RANGE_REF:
-	    case COMPONENT_REF:
 	    case BIT_FIELD_REF:
 	      sflags &= ~OEP_ADDRESS_OF;
 	      break;

On Thu, 12 Nov 2020, Jan Hubicka wrote:

> > On Thu, 12 Nov 2020, Jan Hubicka wrote:
> > 
> > > Hi,
> > > this is updated patch I am re-testing and plan to commit if it suceeds.
> > > 
> > > 	* fold-const.c (operand_compare::operand_equal_p): Compare
> > > 	offsets of fields in component_refs when comparing addresses.
> > > 	(operand_compare::hash_operand): Likewise.
> > > diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> > > index c47557daeba..273ee25ceda 100644
> > > --- a/gcc/fold-const.c
> > > +++ b/gcc/fold-const.c
> > > @@ -3312,11 +3312,36 @@ 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;
> > > -	  flags &= ~OEP_ADDRESS_OF;
> > > -	  return OP_SAME_WITH_NULL (2);
> > > +	  /* 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.  */
> > 
> > looks like you can simplify by doing
> > 
> >           flags &= ~OEP_ADDRESS_OF;
> > 
> > here.  Neither the FIELD_DECL compare nor the offsets need it
> 
> Yep
> > 
> > You elided
> > 
> >           flags &= ~OEP_ADDRESS_OF;
> > -         return OP_SAME_WITH_NULL (2);
> > 
> > that was here when OP_SAME (1), please re-instantiate.
> Sorry for that, that was not very careful.
> Here is updated patch I re-tested x86_64-linux.

OK.

Richard.

> diff --git a/gcc/fold-const.c b/gcc/fold-const.c
> index c47557daeba..ddf18f27cb7 100644
> --- a/gcc/fold-const.c
> +++ b/gcc/fold-const.c
> @@ -3312,10 +3312,32 @@ 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.  */
>  	  flags &= ~OEP_ADDRESS_OF;
> +	  if (!OP_SAME (1))
> +	    {
> +	      if (flags & OEP_ADDRESS_OF)
> +		{
> +		  if (TREE_OPERAND (arg0, 2)
> +		      || TREE_OPERAND (arg1, 2))
> +		    return OP_SAME_WITH_NULL (2);
> +		  tree field0 = TREE_OPERAND (arg0, 1);
> +		  tree field1 = TREE_OPERAND (arg1, 1);
> +
> +		  if (!operand_equal_p (DECL_FIELD_OFFSET (field0),
> +					DECL_FIELD_OFFSET (field1), flags)
> +		      || !operand_equal_p (DECL_FIELD_BIT_OFFSET (field0),
> +					   DECL_FIELD_BIT_OFFSET (field1),
> +					   flags))
> +		    return false;
> +		}
> +	      else
> +		return false;
> +	    }
>  	  return OP_SAME_WITH_NULL (2);
>  
>  	case BIT_FIELD_REF:
> @@ -3787,9 +3809,26 @@ operand_compare::hash_operand (const_tree t, inchash::hash &hstate,
>  	      sflags = flags;
>  	      break;
>  
> +	    case COMPONENT_REF:
> +	      if (sflags & OEP_ADDRESS_OF)
> +		{
> +		  hash_operand (TREE_OPERAND (t, 0), hstate, flags);
> +		  if (TREE_OPERAND (t, 2))
> +		    hash_operand (TREE_OPERAND (t, 2), hstate,
> +				  flags & ~OEP_ADDRESS_OF);
> +		  else
> +		    {
> +		      tree field = TREE_OPERAND (t, 1);
> +		      hash_operand (DECL_FIELD_OFFSET (field),
> +				    hstate, flags & ~OEP_ADDRESS_OF);
> +		      hash_operand (DECL_FIELD_BIT_OFFSET (field),
> +				    hstate, flags & ~OEP_ADDRESS_OF);
> +		    }
> +		  return;
> +		}
> +	      break;
>  	    case ARRAY_REF:
>  	    case ARRAY_RANGE_REF:
> -	    case COMPONENT_REF:
>  	    case BIT_FIELD_REF:
>  	      sflags &= ~OEP_ADDRESS_OF;
>  	      break;
>