Re: Fix MEM_REF creation for shared stack slots

[Richard Biener <rguenther at suse dot de>]

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.