Re: Enable pointer TBAA for LTO

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Mon, Nov 23, 2015 at 12:00:25AM +0100, Jan Hubicka wrote:
> Hi,
> here is updated patch which I finally comitted today.  It addresses all the comments
> and also fixes one nasty bug that really cost me a lot of time to understand. 
> 
> +	  /* LTO type merging does not make any difference between 
> +	     component pointer types.  We may have
> +
> +	     struct foo {int *a;};
> +
> +	     as TYPE_CANONICAL of 
> +
> +	     struct bar {float *a;};
> +
> +	     Because accesses to int * and float * do not alias, we would get
> +	     false negative when accessing the same memory location by
> +	     float ** and bar *. We thus record the canonical type as:
> +
> +	     struct {void *a;};
> +
> +	     void * is special cased and works as a universal pointer type.
> +	     Accesses to it conflicts with accesses to any other pointer
> +	     type.  */
> 
> This problem manifested itself only as a lto-bootstrap miscompare on 32bit
> build and I spent a lot of time localizing the wrong code since it reproduces
> only in quite large programs where we get conficts in canonical type merging
> like this.
> 
> The patch thus updates record_component_aliases to substitute void_ptr_type for
> all pointer types. I re-did the stats.  Now the improvement on dealII is 14%
> that is quite a bit lower than earlier, but still substantial.  Since we have
> voidptr globing counter, I know that the number of disambiguations would go at
> least 19% up if we did not do it.
> 
> THere is a lot of low hanging fruit in that area now, but the real solution is to
> track types that needs to be merge by fortran rules and don't do all this fancy
> globing for C/C++ types.  I will open branch for IPA work and try to prepare this
> for next stage 1.
> 
> bootstrapped/regtested x86_64-linux and ppc64-linux, earlier version tested on i386-linux
> and also on some bigger apps, committed
> 
> Note that we still have bootstrap miscompare with LTO build and --disable-checking,
> I am looking for that now.  Additoinally after fixing the ICEs preventing us to build
> the gnat1 binary, gnat1 aborts. Both these are independent of the patch.
> 
> Honza
> 	* lto.c (iterative_hash_canonical_type): Always recurse for pointers.
> 	(gimple_register_canonical_type_1): Check that pointers do not get
> 	canonical types.
> 	(gimple_register_canonical_type): Do not register pointers.
> 
> 	* tree.c (build_pointer_type_for_mode,build_reference_type_for_mode):
> 	In LTO we do not compute TYPE_CANONICAL of pointers.
> 	(gimple_canonical_types_compatible_p): Improve coments; sanity check
> 	that pointers do not have canonical type that would make us believe
> 	they are different.
> 	* alias.c (get_alias_set): Do structural type equality on pointers;
> 	enable pointer path for LTO; also glob pointer to vector with pointer
> 	to vector element; glob pointers and references for LTO; do more strict
> 	sanity checking about build_pointer_type returning the canonical type
> 	which is also the main variant.
> 	(record_component_aliases): When component type is pointer and we
> 	do LTO; record void_type_node alias set.

...

> Index: alias.c
> ===================================================================
> --- alias.c	(revision 230714)
> +++ alias.c	(working copy)
> @@ -869,13 +869,23 @@ get_alias_set (tree t)
>        set = lang_hooks.get_alias_set (t);
>        if (set != -1)
>  	return set;
> -      return 0;
> +      /* Handle structure type equality for pointer types.  This is easy
> +	 to do, because the code bellow ignore canonical types on these anyway.
> +	 This is important for LTO, where TYPE_CANONICAL for pointers can not
> +	 be meaningfuly computed by the frotnend.  */
> +      if (!POINTER_TYPE_P (t))
> +	{
> +	  /* In LTO we set canonical types for all types where it makes
> +	     sense to do so.  Double check we did not miss some type.  */
> +	  gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t));
> +          return 0;

I have hit this assert on our LTO tests when doing a merge from trunk
to the HSA branch.  On the branch, we generate very simple static
constructors/destructors which just call libgomp (un)registration
routines to which we pass data in static variables of artificial
types.  The assert happens inside varpool_node::finalize_decl calls on
those variables, e.g.:

lto1: internal compiler error: in get_alias_set, at alias.c:880
0x613650 get_alias_set(tree_node*)
        /home/mjambor/gcc/branch/src/gcc/alias.c:880
0x71d2c7 set_mem_attributes_minus_bitpos(rtx_def*, tree_node*, int, long)
        /home/mjambor/gcc/branch/src/gcc/emit-rtl.c:1772
0xd2d2f0 make_decl_rtl(tree_node*)
        /home/mjambor/gcc/branch/src/gcc/varasm.c:1473
0xd310c7 assemble_variable(tree_node*, int, int, int)
        /home/mjambor/gcc/branch/src/gcc/varasm.c:2144
0xd37b32 varpool_node::assemble_decl()
        /home/mjambor/gcc/branch/src/gcc/varpool.c:580
0x6896aa varpool_node::finalize_decl(tree_node*)
        /home/mjambor/gcc/branch/src/gcc/cgraphunit.c:820
0x844da1 hsa_output_kernels
        /home/mjambor/gcc/branch/src/gcc/hsa-brig.c:1954
0x849f04 hsa_output_libgomp_mapping
        /home/mjambor/gcc/branch/src/gcc/hsa-brig.c:2116
0x849f04 hsa_output_brig()
        /home/mjambor/gcc/branch/src/gcc/hsa-brig.c:2356
(hsa_output_brig is called from compile_file in toplev.c)

I am going to commit the following hunk to the branch so that I can
get on with the merge.  If you think it is wrong in any way, please
let me know so that we can find a proper solution.

Thanks,

Martin


--- /home/mjambor/gcc/trunk/src/gcc/alias.c     2015-11-23 11:45:27.850846512 +0100
+++ gcc/alias.c 2015-11-23 14:37:32.098133073 +0100
@@ -877,7 +877,9 @@ get_alias_set (tree t)
        {
          /* In LTO we set canonical types for all types where it makes
             sense to do so.  Double check we did not miss some type.  */
-         gcc_checking_assert (!in_lto_p || !type_with_alias_set_p (t));
+         gcc_checking_assert (!in_lto_p
+                              || !type_with_alias_set_p (t)
+                              || TYPE_ARTIFICIAL (t));
           return 0;
        }
     }


> +	}
> +    }
> +  else
> +    {
> +      t = TYPE_CANONICAL (t);
> +      gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t));
>      }
> -
> -  t = TYPE_CANONICAL (t);
> -
> -  /* The canonical type should not require structural equality checks.  */
> -  gcc_checking_assert (!TYPE_STRUCTURAL_EQUALITY_P (t));
>  
>    /* If this is a type with a known alias set, return it.  */
>    if (TYPE_ALIAS_SET_KNOWN_P (t))

...