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 <rguent...@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;
