| Message ID | 20190601102842.a5u4serztrf4omz7@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Odd behaviour of indirect_ref_may_alias_decl_p in vn oracle | expand |
On Sat, 1 Jun 2019, Jan Hubicka wrote: > Hi, > while looking for the reason for extra disambiguations in > aliasing_component_refs I have noticed an odd case. We newly disambiguate: > > MEM[(Element_t *)_27 + 4B]; > > s.globalIDDataBase_m; > > <mem_ref 0x7fffe2f68f28 > type <integer_type 0x7ffff63ffd20 Element_t sizes-gimplified type_6 SI > size <integer_cst 0x7ffff70ff078 constant 32> > unit-size <integer_cst 0x7ffff70ff090 constant 4> > align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff70fb5e8 precision:32 min <integer_cst 0x7ffff70ff030 -2147483648> max <integer_cst 0x7ffff70ff048 2147483647> > pointer_to_this <pointer_type 0x7fffe55a3dc8>> > tree_0 tree_2 > arg:0 <ssa_name 0x7fffe2f666c0 > type <pointer_type 0x7ffff62df888 type <record_type 0x7ffff658d930 Domain> > public unsigned DI > size <integer_cst 0x7ffff70dee28 constant 64> > unit-size <integer_cst 0x7ffff70dee40 constant 8> > align:64 warn_if_not_align:0 symtab:0 alias-set 216 canonical-type 0x7ffff62df888> > visited > def_stmt _27 = &MEM[(struct OneDomain_t *)&s].D.113982.domain_m[i_26].D.110783.D.44395; > version:27 > ptr-info 0x7ffff5c66540> > arg:1 <integer_cst 0x7fffe98babe8 type <pointer_type 0x7fffe55a3dc8> constant 4>> > <component_ref 0x7ffff1b8d030 > type <pointer_type 0x7ffff494bf18 > type <record_type 0x7ffff494b7e0 GlobalIDDataBase addressable needs-constructing type_1 type_4 type_5 type_6 BLK > size <integer_cst 0x7ffff4fe92d0 constant 576> > unit-size <integer_cst 0x7ffff4fe9048 constant 72> > align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff494b7e0 fields <function_decl 0x7fffecee2000 __dt > context <translation_unit_decl 0x7ffff70eb168 /aux/hubicka/tramp3d-v4b.cpp> > full-name "class GlobalIDDataBase" > needs-constructor needs-destructor X() X(constX&) this=(X&) n_parents=0 use_template=0 interface-unknown > pointer_to_this <pointer_type 0x7ffff494bf18> reference_to_this <reference_type 0x7fffecededc8> chain <type_decl 0x7ffff4948b48 GlobalIDDataBase>> > sizes-gimplified public unsigned type_6 DI > size <integer_cst 0x7ffff70dee28 constant 64> > unit-size <integer_cst 0x7ffff70dee40 constant 8> > align:64 warn_if_not_align:0 symtab:0 alias-set 484 canonical-type 0x7ffff494bf18 > pointer_to_this <pointer_type 0x7fffe4f220a8>> > > arg:0 <mem_ref 0x7fffe2f68fc8 > type <record_type 0x7ffff294d888 INode sizes-gimplified addressable needs-constructing type_1 type_4 type_5 type_6 BLK > size <integer_cst 0x7ffff6f1aed0 constant 320> > unit-size <integer_cst 0x7ffff72d4078 constant 40> > align:64 warn_if_not_align:0 symtab:0 alias-set 604 canonical-type 0x7ffff294d888 fields <const_decl 0x7fffee5b8f50 dimensions> context <translation_unit_decl 0x7ffff70eb168 /aux/hubicka/tramp3d-v4b.cpp> > full-name "class INode<3>" > needs-constructor needs-destructor X() X(constX&) this=(X&) n_parents=0 use_template=1 interface-unknown > pointer_to_this <pointer_type 0x7fffee1a1f18> reference_to_this <reference_type 0x7fffee1a61f8> chain <type_decl 0x7ffff294b850 INode>> > tree_1 tree_2 > arg:0 <addr_expr 0x7ffff56641c0 type <pointer_type 0x7fffee1a1f18> > arg:0 <var_decl 0x7fffec51a5a0 s>> > arg:1 <integer_cst 0x7fffe945f960 constant 0>> > arg:1 <field_decl 0x7fffede17c78 globalIDDataBase_m type <pointer_type 0x7ffff494bf18> > used private unsigned nonlocal decl_3 DI /aux/hubicka/tramp3d-v4b.cpp:5583:21 size <integer_cst 0x7ffff70dee28 64> unit-size <integer_cst 0x7ffff70dee40 8> > align:64 warn_if_not_align:0 offset_align 128 > offset <integer_cst 0x7ffff70dee88 constant 16> bit-offset <integer_cst 0x7ffff70dee28 64> context <record_type 0x7ffff294d888 INode> > chain <field_decl 0x7fffede17d10 key_m type <integer_type 0x7fffede19690 NodeKey_t> > used private nonlocal decl_3 SI /aux/hubicka/tramp3d-v4b.cpp:5584:13 > size <integer_cst 0x7ffff70ff078 constant 32> > unit-size <integer_cst 0x7ffff70ff090 constant 4> > align:32 warn_if_not_align:0 offset_align 128 > offset <integer_cst 0x7ffff70ff288 constant 32> > bit-offset <integer_cst 0x7ffff70deea0 constant 0> context <record_type 0x7ffff294d888 INode> chain <type_decl 0x7fffede17428 INode>>> > /aux/hubicka/tramp3d-v4b.cpp:5457:24 start: /aux/hubicka/tramp3d-v4b.cpp:5457:24 finish: /aux/hubicka/tramp3d-v4b.cpp:5457:24> > > that did not make sense to me becaue ref1 type is integer_type and ref2 is > pointer_type so these should be disambiguated by usual TBAA querry earlier. > > What happens is the following. The analysis happens via vn_reference_lookup > which does > if (! tbaa_p) > r.ref_alias_set = r.base_alias_set = 0; > later this is passed as ref1 but because it reffers to decl the order is > exchanged so it appears as ref2 in in indirect_ref_may_alias_decl_p. > Now there is test: > /* If the alias set for a pointer access is zero all bets are off. */ > if (base1_alias_set == 0) > return true; > which is ok, since base1_alias_set is non-0 (it is coming from indirect_ref). > > I think the code is not supposed to work this way :) > > I not convinced we realy want to give up on TBAA when ref is actual > decl? At least polymorphic call analysis is using the assumption that > placement news do not happen here. > > This patch fixes the odd case in conservative way and also reduces > number of disambiguations noticeably: > > from > aliasing_component_ref_p: 636 disambiguations, 15844 queries > to > aliasing_component_ref_p: 136 disambiguations, 13943 queries > > Can we construct valid placement new testcase where this matters? I think the patch is correct. Consider the decl being accessed via memcpy which will result in base2_alias_set == 0. The GIMPLE memory model says decls are just random storage and thus their dynamic type can change. This is also why there's the "strange" (and also incomplete...) give-ups on "this looks like a view-conversion" are there. Note it's easiest to build GIMPLE testcases for simple disambiguations these days ;) It's also best to quote what we disambiguate by dumping the stmts with TDF_GIMPLE since that shows everything semantically important. But does the patch really make a difference? if (base1_alias_set != base2_alias_set && !alias_sets_conflict_p (base1_alias_set, base2_alias_set)) return false; Should catch this since everything conflicts with zero? So I wonder how your statistcs can be correct? The base1_alias_set == 0 check is redundant as well apart from the case where both are zero which is when we end up skipping the above test. Richard. > Honza > > * tree-ssa-alias.c (indirect_ref_may_alias_decl_p): Also give up > TBAA path when base2_alias_set is 0. > Index: tree-ssa-alias.c > =================================================================== > --- tree-ssa-alias.c (revision 271813) > +++ tree-ssa-alias.c (working copy) > @@ -1295,7 +1296,7 @@ indirect_ref_may_alias_decl_p (tree ref1 > ptrtype1 = TREE_TYPE (TREE_OPERAND (base1, 1)); > > /* If the alias set for a pointer access is zero all bets are off. */ > - if (base1_alias_set == 0) > + if (base1_alias_set == 0 || base2_alias_set == 0) > return true; > > /* When we are trying to disambiguate an access with a pointer dereference >
> I think the patch is correct. Consider the decl being accessed > via memcpy which will result in base2_alias_set == 0. The > GIMPLE memory model says decls are just random storage and thus > their dynamic type can change. This is also why there's the > "strange" (and also incomplete...) give-ups on "this looks like > a view-conversion" are there. In decl variant of oracle we test: /* If the alias set for a pointer access is zero all bets are off. */ if (base1_alias_set == -1) base1_alias_set = get_deref_alias_set (ptrtype1); if (base1_alias_set == 0) return true; if (base2_alias_set == -1) base2_alias_set = get_alias_set (base2); While for two indirect refs we test: /* If the alias set for a pointer access is zero all bets are off. */ if (base1_alias_set == -1) base1_alias_set = get_deref_alias_set (ptrtype1); if (base1_alias_set == 0) return true; if (base2_alias_set == -1) base2_alias_set = get_deref_alias_set (ptrtype2); if (base2_alias_set == 0) return true; Now assume that I have decl of some type, use it for a while and then memcpy completely different data type to it (my understand is that you want to support this in GIMPLE memory model) Now VN sees the original type of decl in the original access. It knows it does not want to use TBAA and to disable it sets base2_alias_set to 0. (It sets base1_alias_set but things are swapped earlier in refs_may_alias_p_1. Now since base1!=0, alias oracle continues. TBAA intersection checks fails, but eventually it succeeds with disambiguation based on access path. I do not think it makes sense to trust access path from the original access to decl when we expect dynamic type to change. For me this has shown as unexpected sucesses of the access path disambiguation which is only possible if alias set check was disabled. > > Note it's easiest to build GIMPLE testcases for simple disambiguations > these days ;) It's also best to quote what we disambiguate by > dumping the stmts with TDF_GIMPLE since that shows everything > semantically important. > > But does the patch really make a difference? > > if (base1_alias_set != base2_alias_set > && !alias_sets_conflict_p (base1_alias_set, base2_alias_set)) > return false; > > Should catch this since everything conflicts with zero? So I > wonder how your statistcs can be correct? The base1_alias_set == 0 > check is redundant as well apart from the case where both > are zero which is when we end up skipping the above test. Everything conflicts with 0 and thus the check fails and oracle continues eventually to aliasing_component_refs which returns false :) Honza > > Richard. > > > > Honza > > > > * tree-ssa-alias.c (indirect_ref_may_alias_decl_p): Also give up > > TBAA path when base2_alias_set is 0. > > Index: tree-ssa-alias.c > > =================================================================== > > --- tree-ssa-alias.c (revision 271813) > > +++ tree-ssa-alias.c (working copy) > > @@ -1295,7 +1296,7 @@ indirect_ref_may_alias_decl_p (tree ref1 > > ptrtype1 = TREE_TYPE (TREE_OPERAND (base1, 1)); > > > > /* If the alias set for a pointer access is zero all bets are off. */ > > - if (base1_alias_set == 0) > > + if (base1_alias_set == 0 || base2_alias_set == 0) > > return true; > > > > /* When we are trying to disambiguate an access with a pointer dereference > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
Hi,
as discussed on IRC today, after all this patch should be correct. I
have re-tested it with x86_64-linux in the following variant which also
moves load of ptrtype1 that is unnecesarily early.
Bootstrapped/regtested x86_64-linux, OK?
* tree-ssa-alias.c (indirect_ref_may_alias_decl_p): Also give up
TBAA path when base2_alias_set is 0.
Index: tree-ssa-alias.c
===================================================================
--- tree-ssa-alias.c (revision 272614)
+++ tree-ssa-alias.c (working copy)
@@ -1458,10 +1466,8 @@ indirect_ref_may_alias_decl_p (tree ref1
if (!flag_strict_aliasing || !tbaa_p)
return true;
- ptrtype1 = TREE_TYPE (TREE_OPERAND (base1, 1));
-
/* If the alias set for a pointer access is zero all bets are off. */
- if (base1_alias_set == 0)
+ if (base1_alias_set == 0 || base2_alias_set == 0)
return true;
/* When we are trying to disambiguate an access with a pointer dereference
@@ -1479,6 +1485,9 @@ indirect_ref_may_alias_decl_p (tree ref1
if (base1_alias_set != base2_alias_set
&& !alias_sets_conflict_p (base1_alias_set, base2_alias_set))
return false;
+
+ ptrtype1 = TREE_TYPE (TREE_OPERAND (base1, 1));
+
/* If the size of the access relevant for TBAA through the pointer
is bigger than the size of the decl we can't possibly access the
decl via that pointer. */
On Mon, 24 Jun 2019, Jan Hubicka wrote: > Hi, > as discussed on IRC today, after all this patch should be correct. I > have re-tested it with x86_64-linux in the following variant which also > moves load of ptrtype1 that is unnecesarily early. > > Bootstrapped/regtested x86_64-linux, OK? OK. Richard. > * tree-ssa-alias.c (indirect_ref_may_alias_decl_p): Also give up > TBAA path when base2_alias_set is 0. > > Index: tree-ssa-alias.c > =================================================================== > --- tree-ssa-alias.c (revision 272614) > +++ tree-ssa-alias.c (working copy) > @@ -1458,10 +1466,8 @@ indirect_ref_may_alias_decl_p (tree ref1 > if (!flag_strict_aliasing || !tbaa_p) > return true; > > - ptrtype1 = TREE_TYPE (TREE_OPERAND (base1, 1)); > - > /* If the alias set for a pointer access is zero all bets are off. */ > - if (base1_alias_set == 0) > + if (base1_alias_set == 0 || base2_alias_set == 0) > return true; > > /* When we are trying to disambiguate an access with a pointer dereference > @@ -1479,6 +1485,9 @@ indirect_ref_may_alias_decl_p (tree ref1 > if (base1_alias_set != base2_alias_set > && !alias_sets_conflict_p (base1_alias_set, base2_alias_set)) > return false; > + > + ptrtype1 = TREE_TYPE (TREE_OPERAND (base1, 1)); > + > /* If the size of the access relevant for TBAA through the pointer > is bigger than the size of the decl we can't possibly access the > decl via that pointer. */
Index: tree-ssa-alias.c =================================================================== --- tree-ssa-alias.c (revision 271813) +++ tree-ssa-alias.c (working copy) @@ -1295,7 +1296,7 @@ indirect_ref_may_alias_decl_p (tree ref1 ptrtype1 = TREE_TYPE (TREE_OPERAND (base1, 1)); /* If the alias set for a pointer access is zero all bets are off. */ - if (base1_alias_set == 0) + if (base1_alias_set == 0 || base2_alias_set == 0) return true; /* When we are trying to disambiguate an access with a pointer dereference
Hi, while looking for the reason for extra disambiguations in aliasing_component_refs I have noticed an odd case. We newly disambiguate: MEM[(Element_t *)_27 + 4B]; s.globalIDDataBase_m; <mem_ref 0x7fffe2f68f28 type <integer_type 0x7ffff63ffd20 Element_t sizes-gimplified type_6 SI size <integer_cst 0x7ffff70ff078 constant 32> unit-size <integer_cst 0x7ffff70ff090 constant 4> align:32 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff70fb5e8 precision:32 min <integer_cst 0x7ffff70ff030 -2147483648> max <integer_cst 0x7ffff70ff048 2147483647> pointer_to_this <pointer_type 0x7fffe55a3dc8>> tree_0 tree_2 arg:0 <ssa_name 0x7fffe2f666c0 type <pointer_type 0x7ffff62df888 type <record_type 0x7ffff658d930 Domain> public unsigned DI size <integer_cst 0x7ffff70dee28 constant 64> unit-size <integer_cst 0x7ffff70dee40 constant 8> align:64 warn_if_not_align:0 symtab:0 alias-set 216 canonical-type 0x7ffff62df888> visited def_stmt _27 = &MEM[(struct OneDomain_t *)&s].D.113982.domain_m[i_26].D.110783.D.44395; version:27 ptr-info 0x7ffff5c66540> arg:1 <integer_cst 0x7fffe98babe8 type <pointer_type 0x7fffe55a3dc8> constant 4>> <component_ref 0x7ffff1b8d030 type <pointer_type 0x7ffff494bf18 type <record_type 0x7ffff494b7e0 GlobalIDDataBase addressable needs-constructing type_1 type_4 type_5 type_6 BLK size <integer_cst 0x7ffff4fe92d0 constant 576> unit-size <integer_cst 0x7ffff4fe9048 constant 72> align:64 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type 0x7ffff494b7e0 fields <function_decl 0x7fffecee2000 __dt > context <translation_unit_decl 0x7ffff70eb168 /aux/hubicka/tramp3d-v4b.cpp> full-name "class GlobalIDDataBase" needs-constructor needs-destructor X() X(constX&) this=(X&) n_parents=0 use_template=0 interface-unknown pointer_to_this <pointer_type 0x7ffff494bf18> reference_to_this <reference_type 0x7fffecededc8> chain <type_decl 0x7ffff4948b48 GlobalIDDataBase>> sizes-gimplified public unsigned type_6 DI size <integer_cst 0x7ffff70dee28 constant 64> unit-size <integer_cst 0x7ffff70dee40 constant 8> align:64 warn_if_not_align:0 symtab:0 alias-set 484 canonical-type 0x7ffff494bf18 pointer_to_this <pointer_type 0x7fffe4f220a8>> arg:0 <mem_ref 0x7fffe2f68fc8 type <record_type 0x7ffff294d888 INode sizes-gimplified addressable needs-constructing type_1 type_4 type_5 type_6 BLK size <integer_cst 0x7ffff6f1aed0 constant 320> unit-size <integer_cst 0x7ffff72d4078 constant 40> align:64 warn_if_not_align:0 symtab:0 alias-set 604 canonical-type 0x7ffff294d888 fields <const_decl 0x7fffee5b8f50 dimensions> context <translation_unit_decl 0x7ffff70eb168 /aux/hubicka/tramp3d-v4b.cpp> full-name "class INode<3>" needs-constructor needs-destructor X() X(constX&) this=(X&) n_parents=0 use_template=1 interface-unknown pointer_to_this <pointer_type 0x7fffee1a1f18> reference_to_this <reference_type 0x7fffee1a61f8> chain <type_decl 0x7ffff294b850 INode>> tree_1 tree_2 arg:0 <addr_expr 0x7ffff56641c0 type <pointer_type 0x7fffee1a1f18> arg:0 <var_decl 0x7fffec51a5a0 s>> arg:1 <integer_cst 0x7fffe945f960 constant 0>> arg:1 <field_decl 0x7fffede17c78 globalIDDataBase_m type <pointer_type 0x7ffff494bf18> used private unsigned nonlocal decl_3 DI /aux/hubicka/tramp3d-v4b.cpp:5583:21 size <integer_cst 0x7ffff70dee28 64> unit-size <integer_cst 0x7ffff70dee40 8> align:64 warn_if_not_align:0 offset_align 128 offset <integer_cst 0x7ffff70dee88 constant 16> bit-offset <integer_cst 0x7ffff70dee28 64> context <record_type 0x7ffff294d888 INode> chain <field_decl 0x7fffede17d10 key_m type <integer_type 0x7fffede19690 NodeKey_t> used private nonlocal decl_3 SI /aux/hubicka/tramp3d-v4b.cpp:5584:13 size <integer_cst 0x7ffff70ff078 constant 32> unit-size <integer_cst 0x7ffff70ff090 constant 4> align:32 warn_if_not_align:0 offset_align 128 offset <integer_cst 0x7ffff70ff288 constant 32> bit-offset <integer_cst 0x7ffff70deea0 constant 0> context <record_type 0x7ffff294d888 INode> chain <type_decl 0x7fffede17428 INode>>> /aux/hubicka/tramp3d-v4b.cpp:5457:24 start: /aux/hubicka/tramp3d-v4b.cpp:5457:24 finish: /aux/hubicka/tramp3d-v4b.cpp:5457:24> that did not make sense to me becaue ref1 type is integer_type and ref2 is pointer_type so these should be disambiguated by usual TBAA querry earlier. What happens is the following. The analysis happens via vn_reference_lookup which does if (! tbaa_p) r.ref_alias_set = r.base_alias_set = 0; later this is passed as ref1 but because it reffers to decl the order is exchanged so it appears as ref2 in in indirect_ref_may_alias_decl_p. Now there is test: /* If the alias set for a pointer access is zero all bets are off. */ if (base1_alias_set == 0) return true; which is ok, since base1_alias_set is non-0 (it is coming from indirect_ref). I think the code is not supposed to work this way :) I not convinced we realy want to give up on TBAA when ref is actual decl? At least polymorphic call analysis is using the assumption that placement news do not happen here. This patch fixes the odd case in conservative way and also reduces number of disambiguations noticeably: from aliasing_component_ref_p: 636 disambiguations, 15844 queries to aliasing_component_ref_p: 136 disambiguations, 13943 queries Can we construct valid placement new testcase where this matters? Honza * tree-ssa-alias.c (indirect_ref_may_alias_decl_p): Also give up TBAA path when base2_alias_set is 0.