| Message ID | 20190521073550.7ne526m5qe777mjh@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
| Series | Fix MEM_REF creation for shared stack slots | expand |
On Tue, 21 May 2019, Jan Hubicka wrote: > Hi, > while creating shared stack slots we create a fake void * pointer and > merge the corresponidng points-to sets. Later ao_ref_from_mem constructs > mem_ref to feed alias oracle from. Since pointer is void we then > dereference it and keep with void_type mem_ref that does not make much sense. > It also makes oracle to punt later on > > /* If either reference is view-converted, give up now. */ > if (same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (ptrtype1)) != 1 > || same_type_for_tbaa (TREE_TYPE (dbase2), TREE_TYPE (base2)) != 1) > return true; > > The patch improves access path disambiguation from: > > refs_may_alias_p: 3027850 disambiguations, 3340416 queries > ref_maybe_used_by_call_p: 6451 disambiguations, 3053430 queries > call_may_clobber_ref_p: 817 disambiguations, 817 queries > aliasing_component_ref_p: 151 disambiguations, 12565 queries > TBAA oracle: 1468434 disambiguations 3010778 queries > 550723 are in alias set 0 > 614261 queries asked about the same object > 0 queries asked about the same alias set > 0 access volatile > 260983 are dependent in the DAG > 116377 are aritificially in conflict with void * > > to: > > Alias oracle query stats: > refs_may_alias_p: 3029219 disambiguations, 3341410 queries > ref_maybe_used_by_call_p: 6451 disambiguations, 3054799 queries > call_may_clobber_ref_p: 817 disambiguations, 817 queries > aliasing_component_ref_p: 1286 disambiguations, 18458 queries > TBAA oracle: 1468536 disambiguations 3013214 queries > 550743 are in alias set 0 > 616203 queries asked about the same object > 0 queries asked about the same alias set > 0 access volatile > 261355 are dependent in the DAG > 116377 are aritificially in conflict with void * > > So about 8times of aliasing_component_refs hitrate. OK, one issue with the patch is that it restores TBAA for the access which we may _not_ do IIRC. > Bootstrapped/regtested x86_64-linux, OK? I'd rather not have that new build_simple_mem_ref_with_type_loc function - the "simple" MEM_REF was to be a way to replace a plain old INDIRECT_REF. So please instead ... > Honza > > * alias.c (ao_ref_from_mem): Use build_simple_mem_ref_with_type. > * tree.c (build_simple_mem_ref_with_type_loc): Break out from ... > (build_simple_mem_ref_loc): ... here. > * fold-const.h (build_simple_mem_ref_with_type_loc): Declare. > (build_simple_mem_ref_with_type): New macro. > Index: alias.c > =================================================================== > --- alias.c (revision 271379) > +++ alias.c (working copy) > @@ -316,7 +316,8 @@ ao_ref_from_mem (ao_ref *ref, const_rtx > { > tree *namep = cfun->gimple_df->decls_to_pointers->get (base); > if (namep) > - ref->base = build_simple_mem_ref (*namep); > + ref->base = build_simple_mem_ref_with_type > + (*namep, build_pointer_type (TREE_TYPE (base))); ... ref->base = build2 (MEM_REF, TREE_TYPE (base), *namep, build_int_cst (TREE_TYPE (*namep), 0)); which preserves TBAA behavior but fixes the 'void' type ref. Thanks, Richard. > } > > ref->ref_alias_set = MEM_ALIAS_SET (mem); > Index: fold-const.h > =================================================================== > --- fold-const.h (revision 271379) > +++ fold-const.h (working copy) > @@ -120,6 +120,9 @@ extern tree fold_indirect_ref_loc (locat > extern tree build_simple_mem_ref_loc (location_t, tree); > #define build_simple_mem_ref(T)\ > build_simple_mem_ref_loc (UNKNOWN_LOCATION, T) > +extern tree build_simple_mem_ref_with_type_loc (location_t, tree, tree); > +#define build_simple_mem_ref_with_type(T, T2)\ > + build_simple_mem_ref_with_type_loc (UNKNOWN_LOCATION, T, T2) > extern poly_offset_int mem_ref_offset (const_tree); > extern tree build_invariant_address (tree, tree, poly_int64); > extern tree constant_boolean_node (bool, tree); > Index: tree.c > =================================================================== > --- tree.c (revision 271402) > +++ tree.c (working copy) > @@ -4907,14 +4907,14 @@ build5 (enum tree_code code, tree tt, tr > } > > /* Build a simple MEM_REF tree with the sematics of a plain INDIRECT_REF > - on the pointer PTR. */ > + on the pointer PTR casted to TYPE. */ > > tree > -build_simple_mem_ref_loc (location_t loc, tree ptr) > +build_simple_mem_ref_with_type_loc (location_t loc, tree ptr, tree ptype) > { > poly_int64 offset = 0; > - tree ptype = TREE_TYPE (ptr); > tree tem; > + gcc_checking_assert (POINTER_TYPE_P (ptype) && ptype != void_type_node); > /* For convenience allow addresses that collapse to a simple base > and offset. */ > if (TREE_CODE (ptr) == ADDR_EXPR > @@ -4938,6 +4938,15 @@ build_simple_mem_ref_loc (location_t loc > return tem; > } > > +/* Build a simple MEM_REF tree with the sematics of a plain INDIRECT_REF > + on the pointer PTR. */ > + > +tree > +build_simple_mem_ref_loc (location_t loc, tree ptr) > +{ > + return build_simple_mem_ref_with_type_loc (loc, ptr, TREE_TYPE (ptr)); > +} > + > /* Return the constant offset of a MEM_REF or TARGET_MEM_REF tree T. */ > > poly_offset_int >
> > So about 8times of aliasing_component_refs hitrate. > > OK, one issue with the patch is that it restores TBAA for the > access which we may _not_ do IIRC. I can see that with stack sharing we have one memory location that for a while is of type A and later is rewritten by type B, but we already give up on optimizing this because of C++ placement new, right? In what scenarios one can disambiguate by the alias set of the reference type (which we do in all cases) but not by the alias set of base type. The code in cfgexpand does not seem to care about either of those. > > > Bootstrapped/regtested x86_64-linux, OK? > > I'd rather not have that new build_simple_mem_ref_with_type_loc > function - the "simple" MEM_REF was to be a way to replace > a plain old INDIRECT_REF. > > So please instead ... > > > Honza > > > > * alias.c (ao_ref_from_mem): Use build_simple_mem_ref_with_type. > > * tree.c (build_simple_mem_ref_with_type_loc): Break out from ... > > (build_simple_mem_ref_loc): ... here. > > * fold-const.h (build_simple_mem_ref_with_type_loc): Declare. > > (build_simple_mem_ref_with_type): New macro. > > Index: alias.c > > =================================================================== > > --- alias.c (revision 271379) > > +++ alias.c (working copy) > > @@ -316,7 +316,8 @@ ao_ref_from_mem (ao_ref *ref, const_rtx > > { > > tree *namep = cfun->gimple_df->decls_to_pointers->get (base); > > if (namep) > > - ref->base = build_simple_mem_ref (*namep); > > + ref->base = build_simple_mem_ref_with_type > > + (*namep, build_pointer_type (TREE_TYPE (base))); > > ... > > ref->base = build2 (MEM_REF, TREE_TYPE (base), *namep, > build_int_cst (TREE_TYPE (*namep), 0)); > > which preserves TBAA behavior but fixes the 'void' type ref. My undrestanding of MEMREF is that it has two types, one is TREE_TYPE (MEMREF) and its ref type taken from TREE_TYPE of the constant. So we will still be dereferencing void which is odd. If globbing is necessary, perhaps the outer type should be somethig like alias set 0 char pointer (used by some builtins such as copysign) or union of all types of vars that gets into a given partition? Honza
On Tue, 21 May 2019, Jan Hubicka wrote: > > > So about 8times of aliasing_component_refs hitrate. > > > > OK, one issue with the patch is that it restores TBAA for the > > access which we may _not_ do IIRC. > > I can see that with stack sharing we have one memory location that for a > while is of type A and later is rewritten by type B, but we already give > up on optimizing this because of C++ placement new, right? > > In what scenarios one can disambiguate by the alias set of the reference > type (which we do in all cases) but not by the alias set of base type. > The code in cfgexpand does not seem to care about either of those. I don't remember exactly but lets change the things independently if possible. > > > > > Bootstrapped/regtested x86_64-linux, OK? > > > > I'd rather not have that new build_simple_mem_ref_with_type_loc > > function - the "simple" MEM_REF was to be a way to replace > > a plain old INDIRECT_REF. > > > > So please instead ... > > > > > Honza > > > > > > * alias.c (ao_ref_from_mem): Use build_simple_mem_ref_with_type. > > > * tree.c (build_simple_mem_ref_with_type_loc): Break out from ... > > > (build_simple_mem_ref_loc): ... here. > > > * fold-const.h (build_simple_mem_ref_with_type_loc): Declare. > > > (build_simple_mem_ref_with_type): New macro. > > > Index: alias.c > > > =================================================================== > > > --- alias.c (revision 271379) > > > +++ alias.c (working copy) > > > @@ -316,7 +316,8 @@ ao_ref_from_mem (ao_ref *ref, const_rtx > > > { > > > tree *namep = cfun->gimple_df->decls_to_pointers->get (base); > > > if (namep) > > > - ref->base = build_simple_mem_ref (*namep); > > > + ref->base = build_simple_mem_ref_with_type > > > + (*namep, build_pointer_type (TREE_TYPE (base))); > > > > ... > > > > ref->base = build2 (MEM_REF, TREE_TYPE (base), *namep, > > build_int_cst (TREE_TYPE (*namep), 0)); > > > > which preserves TBAA behavior but fixes the 'void' type ref. > > > My undrestanding of MEMREF is that it has two types, one is TREE_TYPE > (MEMREF) and its ref type taken from TREE_TYPE of the constant. > So we will still be dereferencing void which is odd. Here we reference 'base' (TREE_TYPE of the mem-ref) and the pointer-type for TBAA purposes is the type of the constant. void * here simply means alias-set zero. > If globbing is necessary, perhaps the outer type should be somethig like > alias set 0 char pointer (used by some builtins such as copysign) or > union of all types of vars that gets into a given partition? Note the original reason might have been latent bugs in the RTL code not correctly dealing with the placement new case. With stack slot sharing you end up with a lot more placement news ;) As said, fixing TREE_TYPE (mem-ref) is quite obvious (it should never have been void but retained the original type of the base). Changing TBAA behavior should be done separately and that should probably simply be ref->base = build2 (MEM_REF, TREE_TYPE (base), *namep, build_int_cst (build_pointer_type (TREE_TYPE (base)), 0); note we retain the original alias-set from RTL: ref->ref_alias_set = MEM_ALIAS_SET (mem); and that might _not_ reflect that of the original tree. For example a MEM[&b, (void * ref-all)0] = 1; may be represented as ref.base = b; ref.ref_alias_set = 0; ref.ref = NULL; losing the ref-all qualification. So it is _not_ easily possible to recreate the original 'base'. There might be code in the component-ref disambiguations looking at those alias-types but that's probably fishy for the cases coming in via ao_ref_from_mem which means being conservative here is important. That may also hold for the type of the reference for ref->base. _Nothing_ should really look at that... In fact the correct type should be salvaged by not doing /* Get the base of the reference and see if we have to reject or adjust it. */ base = ao_ref_base (ref); if (base == NULL_TREE) return false; but doing what ao_ref_base_alias_set does, strip handled-components and thus preserve an eventual inner view-converting MEM_REF for the purpose of building the stack-slot sharing ref... The current (somewhat broken) code simply side-steps this by being awkwardly conservative... So if we want to go full in on "fixing" ref->base (previously we just said doing that is wasted cycles) then do Index: gcc/alias.c =================================================================== --- gcc/alias.c (revision 271415) +++ gcc/alias.c (working copy) @@ -316,7 +316,14 @@ ao_ref_from_mem (ao_ref *ref, const_rtx { tree *namep = cfun->gimple_df->decls_to_pointers->get (base); if (namep) - ref->base = build_simple_mem_ref (*namep); + { + tree orig_base = expr; + while (handled_component_p (orig_base)) + orig_base = TREE_OPERAND (orig_base, 0); + ref->base = build2 (MEM_REF, TREE_TYPE (orig_base), *namep, + build_int_cst + (reference_alias_ptr_type (orig_base), 0)); + } } ref->ref_alias_set = MEM_ALIAS_SET (mem); which preserves all information from original inner MEM_REFs. Richard.
On Tue, 21 May 2019, Richard Biener wrote: > On Tue, 21 May 2019, Jan Hubicka wrote: > > > > > So about 8times of aliasing_component_refs hitrate. > > > > > > OK, one issue with the patch is that it restores TBAA for the > > > access which we may _not_ do IIRC. > > > > I can see that with stack sharing we have one memory location that for a > > while is of type A and later is rewritten by type B, but we already give > > up on optimizing this because of C++ placement new, right? > > > > In what scenarios one can disambiguate by the alias set of the reference > > type (which we do in all cases) but not by the alias set of base type. > > The code in cfgexpand does not seem to care about either of those. > > I don't remember exactly but lets change the things independently > if possible. > > > > > > > > Bootstrapped/regtested x86_64-linux, OK? > > > > > > I'd rather not have that new build_simple_mem_ref_with_type_loc > > > function - the "simple" MEM_REF was to be a way to replace > > > a plain old INDIRECT_REF. > > > > > > So please instead ... > > > > > > > Honza > > > > > > > > * alias.c (ao_ref_from_mem): Use build_simple_mem_ref_with_type. > > > > * tree.c (build_simple_mem_ref_with_type_loc): Break out from ... > > > > (build_simple_mem_ref_loc): ... here. > > > > * fold-const.h (build_simple_mem_ref_with_type_loc): Declare. > > > > (build_simple_mem_ref_with_type): New macro. > > > > Index: alias.c > > > > =================================================================== > > > > --- alias.c (revision 271379) > > > > +++ alias.c (working copy) > > > > @@ -316,7 +316,8 @@ ao_ref_from_mem (ao_ref *ref, const_rtx > > > > { > > > > tree *namep = cfun->gimple_df->decls_to_pointers->get (base); > > > > if (namep) > > > > - ref->base = build_simple_mem_ref (*namep); > > > > + ref->base = build_simple_mem_ref_with_type > > > > + (*namep, build_pointer_type (TREE_TYPE (base))); > > > > > > ... > > > > > > ref->base = build2 (MEM_REF, TREE_TYPE (base), *namep, > > > build_int_cst (TREE_TYPE (*namep), 0)); > > > > > > which preserves TBAA behavior but fixes the 'void' type ref. > > > > > > My undrestanding of MEMREF is that it has two types, one is TREE_TYPE > > (MEMREF) and its ref type taken from TREE_TYPE of the constant. > > So we will still be dereferencing void which is odd. > > Here we reference 'base' (TREE_TYPE of the mem-ref) and the > pointer-type for TBAA purposes is the type of the constant. > void * here simply means alias-set zero. > > > If globbing is necessary, perhaps the outer type should be somethig like > > alias set 0 char pointer (used by some builtins such as copysign) or > > union of all types of vars that gets into a given partition? > > Note the original reason might have been latent bugs in the RTL code > not correctly dealing with the placement new case. With stack slot > sharing you end up with a lot more placement news ;) > > As said, fixing TREE_TYPE (mem-ref) is quite obvious (it should > never have been void but retained the original type of the base). > > Changing TBAA behavior should be done separately and that should > probably simply be > > ref->base = build2 (MEM_REF, TREE_TYPE (base), *namep, > build_int_cst (build_pointer_type (TREE_TYPE (base)), > 0); > > note we retain the original alias-set from RTL: > > ref->ref_alias_set = MEM_ALIAS_SET (mem); > > and that might _not_ reflect that of the original tree. For > example a > > MEM[&b, (void * ref-all)0] = 1; > > may be represented as ref.base = b; ref.ref_alias_set = 0; ref.ref = NULL; > losing the ref-all qualification. So it is _not_ easily possible > to recreate the original 'base'. There might be code in the > component-ref disambiguations looking at those alias-types but that's > probably fishy for the cases coming in via ao_ref_from_mem which > means being conservative here is important. > > That may also hold for the type of the reference for ref->base. > _Nothing_ should really look at that... In fact the correct > type should be salvaged by not doing > > /* Get the base of the reference and see if we have to reject or > adjust it. */ > base = ao_ref_base (ref); > if (base == NULL_TREE) > return false; > > but doing what ao_ref_base_alias_set does, strip handled-components > and thus preserve an eventual inner view-converting MEM_REF for > the purpose of building the stack-slot sharing ref... > > The current (somewhat broken) code simply side-steps this by > being awkwardly conservative... > > So if we want to go full in on "fixing" ref->base (previously > we just said doing that is wasted cycles) then do > > Index: gcc/alias.c > =================================================================== > --- gcc/alias.c (revision 271415) > +++ gcc/alias.c (working copy) > @@ -316,7 +316,14 @@ ao_ref_from_mem (ao_ref *ref, const_rtx > { > tree *namep = cfun->gimple_df->decls_to_pointers->get (base); > if (namep) > - ref->base = build_simple_mem_ref (*namep); > + { > + tree orig_base = expr; > + while (handled_component_p (orig_base)) > + orig_base = TREE_OPERAND (orig_base, 0); > + ref->base = build2 (MEM_REF, TREE_TYPE (orig_base), *namep, > + build_int_cst > + (reference_alias_ptr_type (orig_base), > 0)); > + } > } > > ref->ref_alias_set = MEM_ALIAS_SET (mem); > > > which preserves all information from original inner MEM_REFs. And that should be done at RTL creation time instead of repeatedly over and over. Like with the following. Bootstrap / regtest on x86_64-unknown-linux-gnu in progress. Richard. 2019-05-21 Richard Biener <rguenther@suse.de> * alias.c (ao_ref_from_mem): Move stack-slot sharing rewrite ... * emit-rtl.c (set_mem_attributes_minus_bitpos): ... here. Index: gcc/alias.c =================================================================== --- gcc/alias.c (revision 271463) +++ gcc/alias.c (working copy) @@ -307,18 +307,6 @@ ao_ref_from_mem (ao_ref *ref, const_rtx && TREE_CODE (TMR_BASE (base)) == SSA_NAME))) return false; - /* If this is a reference based on a partitioned decl replace the - base with a MEM_REF of the pointer representative we - created during stack slot partitioning. */ - if (VAR_P (base) - && ! is_global_var (base) - && cfun->gimple_df->decls_to_pointers != NULL) - { - tree *namep = cfun->gimple_df->decls_to_pointers->get (base); - if (namep) - ref->base = build_simple_mem_ref (*namep); - } - ref->ref_alias_set = MEM_ALIAS_SET (mem); /* If MEM_OFFSET or MEM_SIZE are unknown what we got from MEM_EXPR Index: gcc/emit-rtl.c =================================================================== --- gcc/emit-rtl.c (revision 271463) +++ gcc/emit-rtl.c (working copy) @@ -61,6 +61,8 @@ along with GCC; see the file COPYING3. #include "opts.h" #include "predict.h" #include "rtx-vector-builder.h" +#include "gimple.h" +#include "gimple-ssa.h" struct target_rtl default_target_rtl; #if SWITCHABLE_TARGET @@ -2128,6 +2130,26 @@ set_mem_attributes_minus_bitpos (rtx ref apply_bitpos = bitpos; } + /* If this is a reference based on a partitioned decl replace the + base with a MEM_REF of the pointer representative we created + during stack slot partitioning. */ + if (attrs.expr + && VAR_P (base) + && ! is_global_var (base) + && cfun->gimple_df->decls_to_pointers != NULL) + { + tree *namep = cfun->gimple_df->decls_to_pointers->get (base); + if (namep) + { + tree *orig_base = &attrs.expr; + while (handled_component_p (*orig_base)) + orig_base = &TREE_OPERAND (*orig_base, 0); + tree aptrt = reference_alias_ptr_type (*orig_base); + *orig_base = build2 (MEM_REF, TREE_TYPE (*orig_base), *namep, + build_int_cst (aptrt, 0)); + } + } + /* Compute the alignment. */ unsigned int obj_align; unsigned HOST_WIDE_INT obj_bitpos;
> And that should be done at RTL creation time instead of > repeatedly over and over. Like with the following. > > Bootstrap / regtest on x86_64-unknown-linux-gnu in progress. Thanks, for TBAA stats I now get Alias oracle query stats: refs_may_alias_p: 3022975 disambiguations, 3321454 queries ref_maybe_used_by_call_p: 6451 disambiguations, 3048555 queries call_may_clobber_ref_p: 817 disambiguations, 817 queries aliasing_component_ref_p: 187 disambiguations, 16103 queries TBAA oracle: 1452502 disambiguations 2956630 queries 550659 are in alias set 0 576739 queries asked about the same object 0 queries asked about the same alias set 0 access volatile 260391 are dependent in the DAG 116339 are aritificially in conflict with void * So some improvement from original (but less great than with my wrong patch): refs_may_alias_p: 3027850 disambiguations, 3340416 queries ref_maybe_used_by_call_p: 6451 disambiguations, 3053430 queries call_may_clobber_ref_p: 817 disambiguations, 817 queries aliasing_component_ref_p: 151 disambiguations, 12565 queries TBAA oracle: 1468434 disambiguations 3010778 queries 550723 are in alias set 0 614261 queries asked about the same object 0 queries asked about the same alias set 0 access volatile 260983 are dependent in the DAG 116377 are aritificially in conflict with void * Honza > > Richard. > > 2019-05-21 Richard Biener <rguenther@suse.de> > > * alias.c (ao_ref_from_mem): Move stack-slot sharing > rewrite ... > * emit-rtl.c (set_mem_attributes_minus_bitpos): ... here. > > Index: gcc/alias.c > =================================================================== > --- gcc/alias.c (revision 271463) > +++ gcc/alias.c (working copy) > @@ -307,18 +307,6 @@ ao_ref_from_mem (ao_ref *ref, const_rtx > && TREE_CODE (TMR_BASE (base)) == SSA_NAME))) > return false; > > - /* If this is a reference based on a partitioned decl replace the > - base with a MEM_REF of the pointer representative we > - created during stack slot partitioning. */ > - if (VAR_P (base) > - && ! is_global_var (base) > - && cfun->gimple_df->decls_to_pointers != NULL) > - { > - tree *namep = cfun->gimple_df->decls_to_pointers->get (base); > - if (namep) > - ref->base = build_simple_mem_ref (*namep); > - } > - > ref->ref_alias_set = MEM_ALIAS_SET (mem); > > /* If MEM_OFFSET or MEM_SIZE are unknown what we got from MEM_EXPR > Index: gcc/emit-rtl.c > =================================================================== > --- gcc/emit-rtl.c (revision 271463) > +++ gcc/emit-rtl.c (working copy) > @@ -61,6 +61,8 @@ along with GCC; see the file COPYING3. > #include "opts.h" > #include "predict.h" > #include "rtx-vector-builder.h" > +#include "gimple.h" > +#include "gimple-ssa.h" > > struct target_rtl default_target_rtl; > #if SWITCHABLE_TARGET > @@ -2128,6 +2130,26 @@ set_mem_attributes_minus_bitpos (rtx ref > apply_bitpos = bitpos; > } > > + /* If this is a reference based on a partitioned decl replace the > + base with a MEM_REF of the pointer representative we created > + during stack slot partitioning. */ > + if (attrs.expr > + && VAR_P (base) > + && ! is_global_var (base) > + && cfun->gimple_df->decls_to_pointers != NULL) > + { > + tree *namep = cfun->gimple_df->decls_to_pointers->get (base); > + if (namep) > + { > + tree *orig_base = &attrs.expr; > + while (handled_component_p (*orig_base)) > + orig_base = &TREE_OPERAND (*orig_base, 0); > + tree aptrt = reference_alias_ptr_type (*orig_base); > + *orig_base = build2 (MEM_REF, TREE_TYPE (*orig_base), *namep, > + build_int_cst (aptrt, 0)); > + } > + } > + > /* Compute the alignment. */ > unsigned int obj_align; > unsigned HOST_WIDE_INT obj_bitpos;
On 5/21/19 8:37 AM, Jan Hubicka wrote: >> And that should be done at RTL creation time instead of >> repeatedly over and over. Like with the following. >> >> Bootstrap / regtest on x86_64-unknown-linux-gnu in progress. > > Thanks, > for TBAA stats I now get > > Alias oracle query stats: > refs_may_alias_p: 3022975 disambiguations, 3321454 queries > ref_maybe_used_by_call_p: 6451 disambiguations, 3048555 queries > call_may_clobber_ref_p: 817 disambiguations, 817 queries > aliasing_component_ref_p: 187 disambiguations, 16103 queries > TBAA oracle: 1452502 disambiguations 2956630 queries > 550659 are in alias set 0 > 576739 queries asked about the same object > 0 queries asked about the same alias set > 0 access volatile > 260391 are dependent in the DAG > 116339 are aritificially in conflict with void * > > So some improvement from original (but less great than with my wrong > patch): > > refs_may_alias_p: 3027850 disambiguations, 3340416 queries > ref_maybe_used_by_call_p: 6451 disambiguations, 3053430 queries > call_may_clobber_ref_p: 817 disambiguations, 817 queries > aliasing_component_ref_p: 151 disambiguations, 12565 queries > TBAA oracle: 1468434 disambiguations 3010778 queries > 550723 are in alias set 0 > 614261 queries asked about the same object > 0 queries asked about the same alias set > 0 access volatile > 260983 are dependent in the DAG > 116377 are aritificially in conflict with void * My recollection of the stack slot issue is that when objects from different scopes share a slot we can end up reordering their accesses. So we might have { int x; } { long y; } Assume x & y are addressable for some reason. A read of x at the end of the first scope might get moved after a store to y in the second scope because we think the two objects can't alias. IIRC this was particularly problematical with aggressive inlining where stack slot sharing is important to keep stack usage down in the kernel. jeff
On May 21, 2019 5:17:43 PM GMT+02:00, Jeff Law <law@redhat.com> wrote: >On 5/21/19 8:37 AM, Jan Hubicka wrote: >>> And that should be done at RTL creation time instead of >>> repeatedly over and over. Like with the following. >>> >>> Bootstrap / regtest on x86_64-unknown-linux-gnu in progress. >> >> Thanks, >> for TBAA stats I now get >> >> Alias oracle query stats: >> refs_may_alias_p: 3022975 disambiguations, 3321454 queries >> ref_maybe_used_by_call_p: 6451 disambiguations, 3048555 queries >> call_may_clobber_ref_p: 817 disambiguations, 817 queries >> aliasing_component_ref_p: 187 disambiguations, 16103 queries >> TBAA oracle: 1452502 disambiguations 2956630 queries >> 550659 are in alias set 0 >> 576739 queries asked about the same object >> 0 queries asked about the same alias set >> 0 access volatile >> 260391 are dependent in the DAG >> 116339 are aritificially in conflict with void * >> >> So some improvement from original (but less great than with my wrong >> patch): >> >> refs_may_alias_p: 3027850 disambiguations, 3340416 queries >> ref_maybe_used_by_call_p: 6451 disambiguations, 3053430 queries >> call_may_clobber_ref_p: 817 disambiguations, 817 queries >> aliasing_component_ref_p: 151 disambiguations, 12565 queries >> TBAA oracle: 1468434 disambiguations 3010778 queries >> 550723 are in alias set 0 >> 614261 queries asked about the same object >> 0 queries asked about the same alias set >> 0 access volatile >> 260983 are dependent in the DAG >> 116377 are aritificially in conflict with void * >My recollection of the stack slot issue is that when objects from >different scopes share a slot we can end up reordering their accesses. >So we might have > > { > int x; > } > > { > long y; > } > >Assume x & y are addressable for some reason. A read of x at the end >of >the first scope might get moved after a store to y in the second scope >because we think the two objects can't alias. Of course the GCC memory model doesn't allow such code motion. For anti dependences you may not use TBAA. But yes, we had bugs in this area in the past. Richard. > >IIRC this was particularly problematical with aggressive inlining where >stack slot sharing is important to keep stack usage down in the kernel. > >jeff
On Tue, 21 May 2019, Richard Biener wrote: > And that should be done at RTL creation time instead of > repeatedly over and over. Like with the following. > > Bootstrap / regtest on x86_64-unknown-linux-gnu in progress. But we can't get away w/o unsharing the expr we change so committed as follows after Bootstrap / regtest on x86_64-unknown-linux-gnu. Richard. 2019-05-22 Richard Biener <rguenther@suse.de> * alias.c (ao_ref_from_mem): Move stack-slot sharing rewrite ... * emit-rtl.c (set_mem_attributes_minus_bitpos): ... here. Index: gcc/alias.c =================================================================== --- gcc/alias.c (revision 271500) +++ gcc/alias.c (working copy) @@ -307,18 +307,6 @@ ao_ref_from_mem (ao_ref *ref, const_rtx && TREE_CODE (TMR_BASE (base)) == SSA_NAME))) return false; - /* If this is a reference based on a partitioned decl replace the - base with a MEM_REF of the pointer representative we - created during stack slot partitioning. */ - if (VAR_P (base) - && ! is_global_var (base) - && cfun->gimple_df->decls_to_pointers != NULL) - { - tree *namep = cfun->gimple_df->decls_to_pointers->get (base); - if (namep) - ref->base = build_simple_mem_ref (*namep); - } - ref->ref_alias_set = MEM_ALIAS_SET (mem); /* If MEM_OFFSET or MEM_SIZE are unknown what we got from MEM_EXPR Index: gcc/emit-rtl.c =================================================================== --- gcc/emit-rtl.c (revision 271500) +++ gcc/emit-rtl.c (working copy) @@ -61,6 +61,9 @@ along with GCC; see the file COPYING3. #include "opts.h" #include "predict.h" #include "rtx-vector-builder.h" +#include "gimple.h" +#include "gimple-ssa.h" +#include "gimplify.h" struct target_rtl default_target_rtl; #if SWITCHABLE_TARGET @@ -2128,6 +2131,27 @@ set_mem_attributes_minus_bitpos (rtx ref apply_bitpos = bitpos; } + /* If this is a reference based on a partitioned decl replace the + base with a MEM_REF of the pointer representative we created + during stack slot partitioning. */ + if (attrs.expr + && VAR_P (base) + && ! is_global_var (base) + && cfun->gimple_df->decls_to_pointers != NULL) + { + tree *namep = cfun->gimple_df->decls_to_pointers->get (base); + if (namep) + { + attrs.expr = unshare_expr (attrs.expr); + tree *orig_base = &attrs.expr; + while (handled_component_p (*orig_base)) + orig_base = &TREE_OPERAND (*orig_base, 0); + tree aptrt = reference_alias_ptr_type (*orig_base); + *orig_base = build2 (MEM_REF, TREE_TYPE (*orig_base), *namep, + build_int_cst (aptrt, 0)); + } + } + /* Compute the alignment. */ unsigned int obj_align; unsigned HOST_WIDE_INT obj_bitpos;
Index: alias.c =================================================================== --- alias.c (revision 271379) +++ alias.c (working copy) @@ -316,7 +316,8 @@ ao_ref_from_mem (ao_ref *ref, const_rtx { tree *namep = cfun->gimple_df->decls_to_pointers->get (base); if (namep) - ref->base = build_simple_mem_ref (*namep); + ref->base = build_simple_mem_ref_with_type + (*namep, build_pointer_type (TREE_TYPE (base))); } ref->ref_alias_set = MEM_ALIAS_SET (mem); Index: fold-const.h =================================================================== --- fold-const.h (revision 271379) +++ fold-const.h (working copy) @@ -120,6 +120,9 @@ extern tree fold_indirect_ref_loc (locat extern tree build_simple_mem_ref_loc (location_t, tree); #define build_simple_mem_ref(T)\ build_simple_mem_ref_loc (UNKNOWN_LOCATION, T) +extern tree build_simple_mem_ref_with_type_loc (location_t, tree, tree); +#define build_simple_mem_ref_with_type(T, T2)\ + build_simple_mem_ref_with_type_loc (UNKNOWN_LOCATION, T, T2) extern poly_offset_int mem_ref_offset (const_tree); extern tree build_invariant_address (tree, tree, poly_int64); extern tree constant_boolean_node (bool, tree); Index: tree.c =================================================================== --- tree.c (revision 271402) +++ tree.c (working copy) @@ -4907,14 +4907,14 @@ build5 (enum tree_code code, tree tt, tr } /* Build a simple MEM_REF tree with the sematics of a plain INDIRECT_REF - on the pointer PTR. */ + on the pointer PTR casted to TYPE. */ tree -build_simple_mem_ref_loc (location_t loc, tree ptr) +build_simple_mem_ref_with_type_loc (location_t loc, tree ptr, tree ptype) { poly_int64 offset = 0; - tree ptype = TREE_TYPE (ptr); tree tem; + gcc_checking_assert (POINTER_TYPE_P (ptype) && ptype != void_type_node); /* For convenience allow addresses that collapse to a simple base and offset. */ if (TREE_CODE (ptr) == ADDR_EXPR @@ -4938,6 +4938,15 @@ build_simple_mem_ref_loc (location_t loc return tem; } +/* Build a simple MEM_REF tree with the sematics of a plain INDIRECT_REF + on the pointer PTR. */ + +tree +build_simple_mem_ref_loc (location_t loc, tree ptr) +{ + return build_simple_mem_ref_with_type_loc (loc, ptr, TREE_TYPE (ptr)); +} + /* Return the constant offset of a MEM_REF or TARGET_MEM_REF tree T. */ poly_offset_int
Hi, while creating shared stack slots we create a fake void * pointer and merge the corresponidng points-to sets. Later ao_ref_from_mem constructs mem_ref to feed alias oracle from. Since pointer is void we then dereference it and keep with void_type mem_ref that does not make much sense. It also makes oracle to punt later on /* If either reference is view-converted, give up now. */ if (same_type_for_tbaa (TREE_TYPE (base1), TREE_TYPE (ptrtype1)) != 1 || same_type_for_tbaa (TREE_TYPE (dbase2), TREE_TYPE (base2)) != 1) return true; The patch improves access path disambiguation from: refs_may_alias_p: 3027850 disambiguations, 3340416 queries ref_maybe_used_by_call_p: 6451 disambiguations, 3053430 queries call_may_clobber_ref_p: 817 disambiguations, 817 queries aliasing_component_ref_p: 151 disambiguations, 12565 queries TBAA oracle: 1468434 disambiguations 3010778 queries 550723 are in alias set 0 614261 queries asked about the same object 0 queries asked about the same alias set 0 access volatile 260983 are dependent in the DAG 116377 are aritificially in conflict with void * to: Alias oracle query stats: refs_may_alias_p: 3029219 disambiguations, 3341410 queries ref_maybe_used_by_call_p: 6451 disambiguations, 3054799 queries call_may_clobber_ref_p: 817 disambiguations, 817 queries aliasing_component_ref_p: 1286 disambiguations, 18458 queries TBAA oracle: 1468536 disambiguations 3013214 queries 550743 are in alias set 0 616203 queries asked about the same object 0 queries asked about the same alias set 0 access volatile 261355 are dependent in the DAG 116377 are aritificially in conflict with void * So about 8times of aliasing_component_refs hitrate. Bootstrapped/regtested x86_64-linux, OK? Honza * alias.c (ao_ref_from_mem): Use build_simple_mem_ref_with_type. * tree.c (build_simple_mem_ref_with_type_loc): Break out from ... (build_simple_mem_ref_loc): ... here. * fold-const.h (build_simple_mem_ref_with_type_loc): Declare. (build_simple_mem_ref_with_type): New macro.