* Use OEP_ADDRESS_OF in emit-rtl.c @ 2015-10-07 5:29 Jan Hubicka 2015-10-07 8:29 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Jan Hubicka @ 2015-10-07 5:29 UTC (permalink / raw) To: gcc-patches, law, rguenther 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Use OEP_ADDRESS_OF in emit-rtl.c 2015-10-07 5:29 Use OEP_ADDRESS_OF in emit-rtl.c Jan Hubicka @ 2015-10-07 8:29 ` Richard Biener 2015-10-07 17:33 ` Jan Hubicka 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2015-10-07 8:29 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, law 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Use OEP_ADDRESS_OF in emit-rtl.c 2015-10-07 8:29 ` Richard Biener @ 2015-10-07 17:33 ` Jan Hubicka 2015-10-08 7:35 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Jan Hubicka @ 2015-10-07 17:33 UTC (permalink / raw) To: Richard Biener; +Cc: Jan Hubicka, gcc-patches, law > > 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Use OEP_ADDRESS_OF in emit-rtl.c 2015-10-07 17:33 ` Jan Hubicka @ 2015-10-08 7:35 ` Richard Biener 2015-10-08 16:14 ` Jan Hubicka 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2015-10-08 7:35 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, law 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Use OEP_ADDRESS_OF in emit-rtl.c 2015-10-08 7:35 ` Richard Biener @ 2015-10-08 16:14 ` Jan Hubicka 2015-10-09 7:51 ` Richard Biener 0 siblings, 1 reply; 7+ messages in thread From: Jan Hubicka @ 2015-10-08 16:14 UTC (permalink / raw) To: Richard Biener; +Cc: Jan Hubicka, gcc-patches, law > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Use OEP_ADDRESS_OF in emit-rtl.c 2015-10-08 16:14 ` Jan Hubicka @ 2015-10-09 7:51 ` Richard Biener 2015-10-10 20:36 ` Jan Hubicka 0 siblings, 1 reply; 7+ messages in thread From: Richard Biener @ 2015-10-09 7:51 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, law 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Use OEP_ADDRESS_OF in emit-rtl.c 2015-10-09 7:51 ` Richard Biener @ 2015-10-10 20:36 ` Jan Hubicka 0 siblings, 0 replies; 7+ messages in thread From: Jan Hubicka @ 2015-10-10 20:36 UTC (permalink / raw) To: Richard Biener; +Cc: Jan Hubicka, gcc-patches, law > > 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-10-10 20:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-07 5:29 Use OEP_ADDRESS_OF in emit-rtl.c Jan Hubicka 2015-10-07 8:29 ` Richard Biener 2015-10-07 17:33 ` Jan Hubicka 2015-10-08 7:35 ` Richard Biener 2015-10-08 16:14 ` Jan Hubicka 2015-10-09 7:51 ` Richard Biener 2015-10-10 20:36 ` Jan Hubicka
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).