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
>
--
Richard Biener <rguent...@suse.de>
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany;
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah; HRB 21284 (AG Nürnberg)
