Fix MEM_REF creation for shared stack slots

Richard Biener rguenther@suse.de
Tue May 21 13:10:00 GMT 2019


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;



More information about the Gcc-patches mailing list