Use OEP_ADDRESS_OF in emit-rtl.c

Use OEP_ADDRESS_OF in emit-rtl.c

[Jan Hubicka]

Hi,
adding some extra sanity checks to operand_equal_p made me to notice that uses
of operand_equal_p in mem attrs really care about addresses only.  The expression
is tree of the original memory acces MEM RTX was created from and thus the
comparsions should be done with OEP_ADDRESS_OF.

Bootstrapped/regtested x86_64-linux, OK?

Honza

	* emit-rtl.c (mem_attrs_eq_p, mem_expr_equal_p): Pass OEP_ADDRESS_OF
	to operand_equal_p.

Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 228131)
+++ emit-rtl.c	(working copy)
@@ -334,7 +334,7 @@ mem_attrs_eq_p (const struct mem_attrs *
 	  && p->addrspace == q->addrspace
 	  && (p->expr == q->expr
 	      || (p->expr != NULL_TREE && q->expr != NULL_TREE
-		  && operand_equal_p (p->expr, q->expr, 0))));
+		  && operand_equal_p (p->expr, q->expr, OEP_ADDRESS_OF))));
 }
 
 /* Set MEM's memory attributes so that they are the same as ATTRS.  */
@@ -1657,7 +1657,7 @@ mem_expr_equal_p (const_tree expr1, cons
   if (TREE_CODE (expr1) != TREE_CODE (expr2))
     return 0;
 
-  return operand_equal_p (expr1, expr2, 0);
+  return operand_equal_p (expr1, expr2, OEP_ADDRESS_OF);
 }
 
 /* Return OFFSET if XEXP (MEM, 0) - OFFSET is known to be ALIGN

Re: Use OEP_ADDRESS_OF in emit-rtl.c

[Richard Biener]

On Wed, 7 Oct 2015, Jan Hubicka wrote:

> Hi,
> adding some extra sanity checks to operand_equal_p made me to notice that uses
> of operand_equal_p in mem attrs really care about addresses only.  The expression
> is tree of the original memory acces MEM RTX was created from and thus the
> comparsions should be done with OEP_ADDRESS_OF.
> 
> Bootstrapped/regtested x86_64-linux, OK?

Did you audit all callers of mem_attrs_eq_p to see if they really
only care about that?  After all MEM_EXPR, via access paths, encode
type-based alias info and thus replacing one with the other (cse.c use
or ifcvt.c use) is only valid if that doesn't break dependences.

So I don't believe this is safe.

Thanks,
Richard.

> Honza
> 
> 	* emit-rtl.c (mem_attrs_eq_p, mem_expr_equal_p): Pass OEP_ADDRESS_OF
> 	to operand_equal_p.
> 
> Index: emit-rtl.c
> ===================================================================
> --- emit-rtl.c	(revision 228131)
> +++ emit-rtl.c	(working copy)
> @@ -334,7 +334,7 @@ mem_attrs_eq_p (const struct mem_attrs *
>  	  && p->addrspace == q->addrspace
>  	  && (p->expr == q->expr
>  	      || (p->expr != NULL_TREE && q->expr != NULL_TREE
> -		  && operand_equal_p (p->expr, q->expr, 0))));
> +		  && operand_equal_p (p->expr, q->expr, OEP_ADDRESS_OF))));
>  }
>  
>  /* Set MEM's memory attributes so that they are the same as ATTRS.  */
> @@ -1657,7 +1657,7 @@ mem_expr_equal_p (const_tree expr1, cons
>    if (TREE_CODE (expr1) != TREE_CODE (expr2))
>      return 0;
>  
> -  return operand_equal_p (expr1, expr2, 0);
> +  return operand_equal_p (expr1, expr2, OEP_ADDRESS_OF);
>  }
>  
>  /* Return OFFSET if XEXP (MEM, 0) - OFFSET is known to be ALIGN
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Use OEP_ADDRESS_OF in emit-rtl.c

[Jan Hubicka]

> 
> Did you audit all callers of mem_attrs_eq_p to see if they really
> only care about that?  After all MEM_EXPR, via access paths, encode
> type-based alias info and thus replacing one with the other (cse.c use
> or ifcvt.c use) is only valid if that doesn't break dependences.

Hmm, expr is used by ao_ref_from_mem and nonoverlapping_memrefs_p.
The alias set of the access is not taken from expr, but from alias set info
stored in the memory attribute itself (and it is checked by those to match)

I still think it is an address of the expression that matters, not the value.
I think operand_equal_p may, for example, consider two different VAR_DECL equivalent
if their constructors are, because the value is (it doesn't do that), but their
addresses differ.

I will look more into nonoverlapping_memrefs_p and ao_ref_from_mem. The first
one may need some update to tree-alias infrastructure....

Honza
> 
> So I don't believe this is safe.
> 
> Thanks,
> Richard.
> 
> > Honza
> > 
> > 	* emit-rtl.c (mem_attrs_eq_p, mem_expr_equal_p): Pass OEP_ADDRESS_OF
> > 	to operand_equal_p.
> > 
> > Index: emit-rtl.c
> > ===================================================================
> > --- emit-rtl.c	(revision 228131)
> > +++ emit-rtl.c	(working copy)
> > @@ -334,7 +334,7 @@ mem_attrs_eq_p (const struct mem_attrs *
> >  	  && p->addrspace == q->addrspace
> >  	  && (p->expr == q->expr
> >  	      || (p->expr != NULL_TREE && q->expr != NULL_TREE
> > -		  && operand_equal_p (p->expr, q->expr, 0))));
> > +		  && operand_equal_p (p->expr, q->expr, OEP_ADDRESS_OF))));
> >  }
> >  
> >  /* Set MEM's memory attributes so that they are the same as ATTRS.  */
> > @@ -1657,7 +1657,7 @@ mem_expr_equal_p (const_tree expr1, cons
> >    if (TREE_CODE (expr1) != TREE_CODE (expr2))
> >      return 0;
> >  
> > -  return operand_equal_p (expr1, expr2, 0);
> > +  return operand_equal_p (expr1, expr2, OEP_ADDRESS_OF);
> >  }
> >  
> >  /* Return OFFSET if XEXP (MEM, 0) - OFFSET is known to be ALIGN
> > 
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Use OEP_ADDRESS_OF in emit-rtl.c

[Richard Biener]

On Wed, 7 Oct 2015, Jan Hubicka wrote:

> > 
> > Did you audit all callers of mem_attrs_eq_p to see if they really
> > only care about that?  After all MEM_EXPR, via access paths, encode
> > type-based alias info and thus replacing one with the other (cse.c use
> > or ifcvt.c use) is only valid if that doesn't break dependences.
> 
> Hmm, expr is used by ao_ref_from_mem and nonoverlapping_memrefs_p.
> The alias set of the access is not taken from expr, but from alias set info
> stored in the memory attribute itself (and it is checked by those to match)

But the alias-set is not everything and yes, via ao_ref_from_mem MEM_EXPR
"leaks" to the tree oracle which happily calls 
nonoverlapping_component_refs_of_decl_p or nonoverlapping_component_refs_p
on it.

> I still think it is an address of the expression that matters, not the value.
> I think operand_equal_p may, for example, consider two different VAR_DECL equivalent
> if their constructors are, because the value is (it doesn't do that), but their
> addresses differ.

It's structural equality of the MEM_EXPR that matters.  That neither
operand_equal_p (..., 0) nor operand_equal_p (..., OEP_ADDRESS_OF) is
an exact implementation for this (well, I think with '0' flags it was
designed to be this, at least for memory references) is of course
suspicious.  But that doesn't make using OEP_ADDRESS_OF the correct thing
to do.

> I will look more into nonoverlapping_memrefs_p and ao_ref_from_mem. The first
> one may need some update to tree-alias infrastructure....

I'd rather remove it completely (at least that was my plan eventually).
rtx_refs_may_alias_p is supposed to handle everything it handles.

Richard.

> Honza
> > 
> > So I don't believe this is safe.
> > 
> > Thanks,
> > Richard.
> > 
> > > Honza
> > > 
> > > 	* emit-rtl.c (mem_attrs_eq_p, mem_expr_equal_p): Pass OEP_ADDRESS_OF
> > > 	to operand_equal_p.
> > > 
> > > Index: emit-rtl.c
> > > ===================================================================
> > > --- emit-rtl.c	(revision 228131)
> > > +++ emit-rtl.c	(working copy)
> > > @@ -334,7 +334,7 @@ mem_attrs_eq_p (const struct mem_attrs *
> > >  	  && p->addrspace == q->addrspace
> > >  	  && (p->expr == q->expr
> > >  	      || (p->expr != NULL_TREE && q->expr != NULL_TREE
> > > -		  && operand_equal_p (p->expr, q->expr, 0))));
> > > +		  && operand_equal_p (p->expr, q->expr, OEP_ADDRESS_OF))));
> > >  }
> > >  
> > >  /* Set MEM's memory attributes so that they are the same as ATTRS.  */
> > > @@ -1657,7 +1657,7 @@ mem_expr_equal_p (const_tree expr1, cons
> > >    if (TREE_CODE (expr1) != TREE_CODE (expr2))
> > >      return 0;
> > >  
> > > -  return operand_equal_p (expr1, expr2, 0);
> > > +  return operand_equal_p (expr1, expr2, OEP_ADDRESS_OF);
> > >  }
> > >  
> > >  /* Return OFFSET if XEXP (MEM, 0) - OFFSET is known to be ALIGN
> > > 
> > > 
> > 
> > -- 
> > Richard Biener <rguenther@suse.de>
> > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Use OEP_ADDRESS_OF in emit-rtl.c

[Jan Hubicka]

> On Wed, 7 Oct 2015, Jan Hubicka wrote:
> 
> > > 
> > > Did you audit all callers of mem_attrs_eq_p to see if they really
> > > only care about that?  After all MEM_EXPR, via access paths, encode
> > > type-based alias info and thus replacing one with the other (cse.c use
> > > or ifcvt.c use) is only valid if that doesn't break dependences.
> > 
> > Hmm, expr is used by ao_ref_from_mem and nonoverlapping_memrefs_p.
> > The alias set of the access is not taken from expr, but from alias set info
> > stored in the memory attribute itself (and it is checked by those to match)
> 
> But the alias-set is not everything and yes, via ao_ref_from_mem MEM_EXPR
> "leaks" to the tree oracle which happily calls 
> nonoverlapping_component_refs_of_decl_p or nonoverlapping_component_refs_p
> on it.
> 
> > I still think it is an address of the expression that matters, not the value.
> > I think operand_equal_p may, for example, consider two different VAR_DECL equivalent
> > if their constructors are, because the value is (it doesn't do that), but their
> > addresses differ.
> 
> It's structural equality of the MEM_EXPR that matters.  That neither
> operand_equal_p (..., 0) nor operand_equal_p (..., OEP_ADDRESS_OF) is
> an exact implementation for this (well, I think with '0' flags it was
> designed to be this, at least for memory references) is of course
> suspicious.  But that doesn't make using OEP_ADDRESS_OF the correct thing
> to do.

Hmm, I see.  I wonder how complex the expressions are and if we can't simply
compare AO properties of MEM_REF at toplevel and then dispatch to
operand_equal_p (..., OEP_ADDRESS_OF)
which would make more sense to me.

I would basically expect decls and mem_refs here.  Reason why I started to look
into that is that I added sanity check that operand_equal_p (...,0) is not called
on things that do not have value (function types and incomplete types) and this
is one of places that fires.

> 
> > I will look more into nonoverlapping_memrefs_p and ao_ref_from_mem. The first
> > one may need some update to tree-alias infrastructure....
> 
> I'd rather remove it completely (at least that was my plan eventually).
> rtx_refs_may_alias_p is supposed to handle everything it handles.

Yep, that was my feeling from looking into that yesterday....

Honza

Re: Use OEP_ADDRESS_OF in emit-rtl.c

[Richard Biener]

On Thu, 8 Oct 2015, Jan Hubicka wrote:

> > On Wed, 7 Oct 2015, Jan Hubicka wrote:
> > 
> > > > 
> > > > Did you audit all callers of mem_attrs_eq_p to see if they really
> > > > only care about that?  After all MEM_EXPR, via access paths, encode
> > > > type-based alias info and thus replacing one with the other (cse.c use
> > > > or ifcvt.c use) is only valid if that doesn't break dependences.
> > > 
> > > Hmm, expr is used by ao_ref_from_mem and nonoverlapping_memrefs_p.
> > > The alias set of the access is not taken from expr, but from alias set info
> > > stored in the memory attribute itself (and it is checked by those to match)
> > 
> > But the alias-set is not everything and yes, via ao_ref_from_mem MEM_EXPR
> > "leaks" to the tree oracle which happily calls 
> > nonoverlapping_component_refs_of_decl_p or nonoverlapping_component_refs_p
> > on it.
> > 
> > > I still think it is an address of the expression that matters, not the value.
> > > I think operand_equal_p may, for example, consider two different VAR_DECL equivalent
> > > if their constructors are, because the value is (it doesn't do that), but their
> > > addresses differ.
> > 
> > It's structural equality of the MEM_EXPR that matters.  That neither
> > operand_equal_p (..., 0) nor operand_equal_p (..., OEP_ADDRESS_OF) is
> > an exact implementation for this (well, I think with '0' flags it was
> > designed to be this, at least for memory references) is of course
> > suspicious.  But that doesn't make using OEP_ADDRESS_OF the correct thing
> > to do.
> 
> Hmm, I see.  I wonder how complex the expressions are and if we can't simply
> compare AO properties of MEM_REF at toplevel and then dispatch to
> operand_equal_p (..., OEP_ADDRESS_OF)
> which would make more sense to me.

I can't see how we can do that comparison conservatively without resorting
to exact equality.  The reason is that the two MEM_EXPRs e1 and e2 have
to behave equal TBAA-wise when compared against arbitrary e3.  We
can be sure this is the case if e1 and e2 have alias-set zero or if
they are structurally equivalent.  But that's all I think.

> I would basically expect decls and mem_refs here.  Reason why I started 
> to look into that is that I added sanity check that operand_equal_p 
> (...,0) is not called on things that do not have value (function types 
> and incomplete types) and this is one of places that fires.

MEM_EXPR can be arbitrarily complex (including array-refs with
variable index SSA name use for example).

> > 
> > > I will look more into nonoverlapping_memrefs_p and ao_ref_from_mem. The first
> > > one may need some update to tree-alias infrastructure....
> > 
> > I'd rather remove it completely (at least that was my plan eventually).
> > rtx_refs_may_alias_p is supposed to handle everything it handles.
> 
> Yep, that was my feeling from looking into that yesterday....

So the experiment to carry out is (same for all other callers)

Index: gcc/alias.c
===================================================================
--- gcc/alias.c (revision 228597)
+++ gcc/alias.c (working copy)
@@ -2710,10 +2710,10 @@ true_dependence_1 (const_rtx mem, machin
   if (mems_in_disjoint_alias_sets_p (x, mem))
     return 0;
 
-  if (nonoverlapping_memrefs_p (mem, x, false))
-    return 0;
-
-  return rtx_refs_may_alias_p (x, mem, true);
+  int tem = rtx_refs_may_alias_p (x, mem, true);
+  if (tem)
+    gcc_assert (!nonoverlapping_memrefs_p (mem, x, false));
+  return tem;
 }
 
 /* True dependence: X is read after store in MEM takes place.  */

and analyze what fires (or hope nothing does).

Richard.

> Honza
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Use OEP_ADDRESS_OF in emit-rtl.c

[Jan Hubicka]

> > Hmm, I see.  I wonder how complex the expressions are and if we can't simply
> > compare AO properties of MEM_REF at toplevel and then dispatch to
> > operand_equal_p (..., OEP_ADDRESS_OF)
> > which would make more sense to me.
> 
> I can't see how we can do that comparison conservatively without resorting
> to exact equality.  The reason is that the two MEM_EXPRs e1 and e2 have
> to behave equal TBAA-wise when compared against arbitrary e3.  We

This is not very different from what we do at gimple level i.e. in tail merging,
ipa-icf or load/store motion.  Maybe we want something like OEP_ALIAS or OEP_STRUCTURAL
flag to control this...

> can be sure this is the case if e1 and e2 have alias-set zero or if
> they are structurally equivalent.  But that's all I think.
> 
> > I would basically expect decls and mem_refs here.  Reason why I started 
> > to look into that is that I added sanity check that operand_equal_p 
> > (...,0) is not called on things that do not have value (function types 
> > and incomplete types) and this is one of places that fires.
> 
> MEM_EXPR can be arbitrarily complex (including array-refs with
> variable index SSA name use for example).
> 
> > > 
> > > > I will look more into nonoverlapping_memrefs_p and ao_ref_from_mem. The first
> > > > one may need some update to tree-alias infrastructure....
> > > 
> > > I'd rather remove it completely (at least that was my plan eventually).
> > > rtx_refs_may_alias_p is supposed to handle everything it handles.
> > 
> > Yep, that was my feeling from looking into that yesterday....
> 
> So the experiment to carry out is (same for all other callers)
> 
> Index: gcc/alias.c
> ===================================================================
> --- gcc/alias.c (revision 228597)
> +++ gcc/alias.c (working copy)
> @@ -2710,10 +2710,10 @@ true_dependence_1 (const_rtx mem, machin
>    if (mems_in_disjoint_alias_sets_p (x, mem))
>      return 0;
>  
> -  if (nonoverlapping_memrefs_p (mem, x, false))
> -    return 0;
> -
> -  return rtx_refs_may_alias_p (x, mem, true);
> +  int tem = rtx_refs_may_alias_p (x, mem, true);
> +  if (tem)
> +    gcc_assert (!nonoverlapping_memrefs_p (mem, x, false));
> +  return tem;
>  }
>  
>  /* True dependence: X is read after store in MEM takes place.  */
> 
> and analyze what fires (or hope nothing does).

OK, will give it a try...
Honza
> 
> Richard.
> 
> > Honza
> > 
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)