Re: [PATCH] Fix PR38151 and a load of alias problems

[Richard Guenther <rguenther at suse dot de>]

On Sun, 23 Nov 2008, Richard Guenther wrote:

> 
> This fixes PR38151 by properly using a ref-all pointer in
> ix86_gimplify_va_arg to write to the temporary that is then
> read as a structure.
> 
> This bug exposes several bugs in the way we handle type-based
> aliasing with ref-all and DECL_POINTER_ALIAS_SET pointers and
> also with setting up the aliases for SMTs.  First we shouldn't
> do TBAA pruning for those pointers, second we need to build
> proper SMTs for such pointers, not only looking at their types
> alias-set.  Third the optimization we have in place to optimize
> the number of aliases for an SMT misses an important fact that
> gets exposed with this testcase - we do not account for writes
> to parts of a structure which will use a separate SMT.
> 
> I actually tested a different patch which makes all accesses
> produced by ix86_gimplify_va_arg ref-all which doesn't need the
> third bug fixed.  This is the second patch after the first one
> which is still in testing.
> 
> I believe the first patch is correct and necessary - any other
> opinions?

Danny noticed the tree-ssa-structalias.c hunk is redundant, so
here's the patch without that and with a testcase for issue three.

Bootstrapped and tested on x86_64-unknown-linux-gnu, I'll apply that
tomorrow.

Richard.

2008-11-22  Richard Guenther  <rguenther@suse.de>

	PR middle-end/38151
	PR middle-end/38236
	* tree-ssa-alias.c (struct alias_info): Remove written_vars.
	Remove dereferenced_ptrs_store and dereferenced_ptrs_load
	in favor of dereferenced_ptrs.
	(init_alias_info): Adjust.
	(delete_alias_info): Likewise.
	(compute_flow_insensitive_aliasing): Properly
	include all aliased variables.
	(update_alias_info_1): Use dereferenced_ptrs.
	(setup_pointers_and_addressables): Likewise.
	(get_smt_for): Honor ref-all pointers and pointers with known alias
	set properly.
	* config/i386/i386.c (ix86_gimplify_va_arg): Use ref-all pointers.

	* gcc.c-torture/execute/pr38151.c: New testcase.
	* gcc.c-torture/execute/pr38236.c: Likewise.

Index: gcc/tree-ssa-alias.c
===================================================================
*** gcc/tree-ssa-alias.c	(revision 142124)
--- gcc/tree-ssa-alias.c	(working copy)
*************** struct alias_info
*** 188,202 ****
    struct alias_map_d **pointers;
    size_t num_pointers;
  
!   /* Variables that have been written to directly (i.e., not through a
!      pointer dereference).  */
!   struct pointer_set_t *written_vars;
! 
!   /* Pointers that have been used in an indirect store operation.  */
!   struct pointer_set_t *dereferenced_ptrs_store;
! 
!   /* Pointers that have been used in an indirect load operation.  */
!   struct pointer_set_t *dereferenced_ptrs_load;
  };
  
  
--- 188,195 ----
    struct alias_map_d **pointers;
    size_t num_pointers;
  
!   /* Pointers that have been used in an indirect load/store operation.  */
!   struct pointer_set_t *dereferenced_ptrs;
  };
  
  
*************** init_alias_info (void)
*** 2073,2081 ****
    ai->ssa_names_visited = sbitmap_alloc (num_ssa_names);
    sbitmap_zero (ai->ssa_names_visited);
    ai->processed_ptrs = VEC_alloc (tree, heap, 50);
!   ai->written_vars = pointer_set_create ();
!   ai->dereferenced_ptrs_store = pointer_set_create ();
!   ai->dereferenced_ptrs_load = pointer_set_create ();
  
    /* Clear out all memory reference stats.  */
    init_mem_ref_stats ();
--- 2066,2072 ----
    ai->ssa_names_visited = sbitmap_alloc (num_ssa_names);
    sbitmap_zero (ai->ssa_names_visited);
    ai->processed_ptrs = VEC_alloc (tree, heap, 50);
!   ai->dereferenced_ptrs = pointer_set_create ();
  
    /* Clear out all memory reference stats.  */
    init_mem_ref_stats ();
*************** delete_alias_info (struct alias_info *ai
*** 2123,2131 ****
      free (ai->pointers[i]);
    free (ai->pointers);
  
!   pointer_set_destroy (ai->written_vars);
!   pointer_set_destroy (ai->dereferenced_ptrs_store);
!   pointer_set_destroy (ai->dereferenced_ptrs_load);
    free (ai);
  
    delete_mem_ref_stats (cfun);
--- 2114,2120 ----
      free (ai->pointers[i]);
    free (ai->pointers);
  
!   pointer_set_destroy (ai->dereferenced_ptrs);
    free (ai);
  
    delete_mem_ref_stats (cfun);
*************** compute_flow_insensitive_aliasing (struc
*** 2361,2383 ****
  	{
  	  struct alias_map_d *v_map;
  	  var_ann_t v_ann;
- 	  bool tag_stored_p, var_stored_p;
  	  
  	  v_map = ai->addressable_vars[j];
  	  var = v_map->var;
  	  v_ann = var_ann (var);
  
! 	  /* Skip memory tags and variables that have never been
! 	     written to.  We also need to check if the variables are
! 	     call-clobbered because they may be overwritten by
! 	     function calls.  */
! 	  tag_stored_p = pointer_set_contains (ai->written_vars, tag)
! 	                 || is_call_clobbered (tag);
! 	  var_stored_p = pointer_set_contains (ai->written_vars, var)
! 	                 || is_call_clobbered (var);
! 	  if (!tag_stored_p && !var_stored_p)
! 	    continue;
! 	     
  	  if (may_alias_p (p_map->var, p_map->set, var, v_map->set, false))
  	    {
  	      /* Add VAR to TAG's may-aliases set.  */
--- 2350,2367 ----
  	{
  	  struct alias_map_d *v_map;
  	  var_ann_t v_ann;
  	  
  	  v_map = ai->addressable_vars[j];
  	  var = v_map->var;
  	  v_ann = var_ann (var);
  
! 	  /* We used to skip variables that have never been written to
! 	     if the memory tag has been never written to directly (or
! 	     either of them were call clobbered).  This is not enough
! 	     though, as this misses writes through the tags aliases.
! 	     So, for correctness we need to include any aliased
! 	     variable here.  */
! 
  	  if (may_alias_p (p_map->var, p_map->set, var, v_map->set, false))
  	    {
  	      /* Add VAR to TAG's may-aliases set.  */
*************** update_alias_info_1 (gimple stmt, struct
*** 2618,2630 ****
  	  /* ???  For always executed direct dereferences we can
  	     apply TBAA-pruning to their escape set.  */
  
! 	  /* If this is a store operation, mark OP as being
! 	     dereferenced to store, otherwise mark it as being
! 	     dereferenced to load.  */
! 	  if (num_stores > 0)
! 	    pointer_set_insert (ai->dereferenced_ptrs_store, var);
! 	  else
! 	    pointer_set_insert (ai->dereferenced_ptrs_load, var);
  
  	  /* Update the frequency estimate for all the dereferences of
  	     pointer OP.  */
--- 2602,2609 ----
  	  /* ???  For always executed direct dereferences we can
  	     apply TBAA-pruning to their escape set.  */
  
! 	  /* Mark OP as being dereferenced.  */
! 	  pointer_set_insert (ai->dereferenced_ptrs, var);
  
  	  /* Update the frequency estimate for all the dereferences of
  	     pointer OP.  */
*************** update_alias_info_1 (gimple stmt, struct
*** 2649,2655 ****
  	  if (is_gimple_call (stmt)
  	      || stmt_escape_type == ESCAPE_STORED_IN_GLOBAL)
  	    {
! 	      pointer_set_insert (ai->dereferenced_ptrs_store, var);
  	      pi->memory_tag_needed = 1;
  	    }
  	}
--- 2628,2634 ----
  	  if (is_gimple_call (stmt)
  	      || stmt_escape_type == ESCAPE_STORED_IN_GLOBAL)
  	    {
! 	      pointer_set_insert (ai->dereferenced_ptrs, var);
  	      pi->memory_tag_needed = 1;
  	    }
  	}
*************** update_alias_info_1 (gimple stmt, struct
*** 2667,2683 ****
  
        mem_ref_stats->num_mem_stmts++;
  
-       /* Add all decls written to to the list of written variables.  */
-       if (gimple_has_lhs (stmt)
- 	  && TREE_CODE (gimple_get_lhs (stmt)) != SSA_NAME)
- 	{
- 	  tree lhs = gimple_get_lhs (stmt);
- 	  while (handled_component_p (lhs))
- 	    lhs = TREE_OPERAND (lhs, 0);
- 	  if (DECL_P (lhs))
- 	    pointer_set_insert (ai->written_vars, lhs);
- 	}
- 
        /* Notice that we only update memory reference stats for symbols
  	 loaded and stored by the statement if the statement does not
  	 contain pointer dereferences and it is not a call/asm site.
--- 2646,2651 ----
*************** setup_pointers_and_addressables (struct
*** 2770,2776 ****
  	  /* Since we don't keep track of volatile variables, assume that
  	     these pointers are used in indirect store operations.  */
  	  if (TREE_THIS_VOLATILE (var))
! 	    pointer_set_insert (ai->dereferenced_ptrs_store, var);
  
  	  num_pointers++;
  	}
--- 2738,2744 ----
  	  /* Since we don't keep track of volatile variables, assume that
  	     these pointers are used in indirect store operations.  */
  	  if (TREE_THIS_VOLATILE (var))
! 	    pointer_set_insert (ai->dereferenced_ptrs, var);
  
  	  num_pointers++;
  	}
*************** setup_pointers_and_addressables (struct
*** 2845,2852 ****
           array and create a symbol memory tag for them.  */
        if (POINTER_TYPE_P (TREE_TYPE (var)))
  	{
! 	  if ((pointer_set_contains (ai->dereferenced_ptrs_store, var)
! 	       || pointer_set_contains (ai->dereferenced_ptrs_load, var)))
  	    {
  	      tree tag, old_tag;
  	      var_ann_t t_ann;
--- 2813,2819 ----
           array and create a symbol memory tag for them.  */
        if (POINTER_TYPE_P (TREE_TYPE (var)))
  	{
! 	  if (pointer_set_contains (ai->dereferenced_ptrs, var))
  	    {
  	      tree tag, old_tag;
  	      var_ann_t t_ann;
*************** setup_pointers_and_addressables (struct
*** 2872,2882 ****
  
  	      /* Associate the tag with pointer VAR.  */
  	      set_symbol_mem_tag (var, tag);
- 
- 	      /* If pointer VAR has been used in a store operation,
- 		 then its memory tag must be marked as written-to.  */
- 	      if (pointer_set_contains (ai->dereferenced_ptrs_store, var))
- 		pointer_set_insert (ai->written_vars, tag);
  	    }
  	  else
  	    {
--- 2839,2844 ----
*************** get_smt_for (tree ptr, struct alias_info
*** 3298,3304 ****
    size_t i;
    tree tag;
    tree tag_type = TREE_TYPE (TREE_TYPE (ptr));
!   alias_set_type tag_set = get_alias_set (tag_type);
  
    /* To avoid creating unnecessary memory tags, only create one memory tag
       per alias set class.  Note that it may be tempting to group
--- 3260,3281 ----
    size_t i;
    tree tag;
    tree tag_type = TREE_TYPE (TREE_TYPE (ptr));
!   alias_set_type tag_set;
! 
!   /* Get the alias set to be used for the pointed-to memory.  If that
!      differs from what we would get from looking at the type adjust
!      the tag_type to void to make sure we get a proper alias set from
!      just looking at the SMT we create.  */
!   tag_set = get_alias_set (tag_type);
!   if (TYPE_REF_CAN_ALIAS_ALL (TREE_TYPE (ptr))
!       /* This is overly conservative but we do not want to assign
!          restrict alias sets here (which if they are not assigned
!          are -2 but still "known").  */
!       || DECL_POINTER_ALIAS_SET_KNOWN_P (ptr))
!     {
!       tag_set = 0;
!       tag_type = void_type_node;
!     }
  
    /* To avoid creating unnecessary memory tags, only create one memory tag
       per alias set class.  Note that it may be tempting to group
*************** get_smt_for (tree ptr, struct alias_info
*** 3329,3335 ****
  	 artificial variable representing the memory location
  	 pointed-to by PTR.  */
        tag = symbol_mem_tag (ptr);
!       if (tag == NULL_TREE)
  	tag = create_memory_tag (tag_type, true);
  
        /* Add PTR to the POINTERS array.  Note that we are not interested in
--- 3306,3313 ----
  	 artificial variable representing the memory location
  	 pointed-to by PTR.  */
        tag = symbol_mem_tag (ptr);
!       if (tag == NULL_TREE
! 	  || tag_set != get_alias_set (tag))
  	tag = create_memory_tag (tag_type, true);
  
        /* Add PTR to the POINTERS array.  Note that we are not interested in
Index: gcc/config/i386/i386.c
===================================================================
*** gcc/config/i386/i386.c	(revision 142124)
--- gcc/config/i386/i386.c	(working copy)
*************** ix86_gimplify_va_arg (tree valist, tree
*** 6753,6758 ****
--- 6753,6760 ----
  	      enum machine_mode mode = GET_MODE (reg);
  	      tree piece_type = lang_hooks.types.type_for_mode (mode, 1);
  	      tree addr_type = build_pointer_type (piece_type);
+ 	      tree daddr_type = build_pointer_type_for_mode (piece_type,
+ 							     ptr_mode, true);
  	      tree src_addr, src;
  	      int src_offset;
  	      tree dest_addr, dest;
*************** ix86_gimplify_va_arg (tree valist, tree
*** 6772,6779 ****
  				      size_int (src_offset));
  	      src = build_va_arg_indirect_ref (src_addr);
  
! 	      dest_addr = fold_convert (addr_type, addr);
! 	      dest_addr = fold_build2 (POINTER_PLUS_EXPR, addr_type, dest_addr,
  				       size_int (INTVAL (XEXP (slot, 1))));
  	      dest = build_va_arg_indirect_ref (dest_addr);
  
--- 6774,6781 ----
  				      size_int (src_offset));
  	      src = build_va_arg_indirect_ref (src_addr);
  
! 	      dest_addr = fold_convert (daddr_type, addr);
! 	      dest_addr = fold_build2 (POINTER_PLUS_EXPR, daddr_type, dest_addr,
  				       size_int (INTVAL (XEXP (slot, 1))));
  	      dest = build_va_arg_indirect_ref (dest_addr);
  
Index: gcc/testsuite/gcc.c-torture/execute/pr38151.c
===================================================================
*** gcc/testsuite/gcc.c-torture/execute/pr38151.c	(revision 0)
--- gcc/testsuite/gcc.c-torture/execute/pr38151.c	(revision 0)
***************
*** 0 ****
--- 1,46 ----
+ void abort (void);
+ 
+ struct S2848
+ {
+   unsigned int a;
+   _Complex int b;
+   struct
+   {
+   } __attribute__ ((aligned)) c;
+ };
+ 
+ struct S2848 s2848;
+ 
+ int fails;
+ 
+ void  __attribute__((noinline))
+ check2848va (int z, ...)
+ {
+   struct S2848 arg;
+   __builtin_va_list ap;
+ 
+   __builtin_va_start (ap, z);
+ 
+   arg = __builtin_va_arg (ap, struct S2848);
+ 
+   if (s2848.a != arg.a)
+     ++fails;
+   if (s2848.b != arg.b)
+     ++fails;
+ 
+   __builtin_va_end (ap);
+ }
+ 
+ int main (void)
+ {
+   s2848.a = 4027477739U;
+   s2848.b = (723419448 + -218144346 * __extension__ 1i);
+ 
+   check2848va (1, s2848);
+ 
+   if (fails)
+     abort ();
+ 
+   return 0;
+ }
+ 
Index: gcc/testsuite/gcc.c-torture/execute/pr38236.c
===================================================================
*** gcc/testsuite/gcc.c-torture/execute/pr38236.c	(revision 0)
--- gcc/testsuite/gcc.c-torture/execute/pr38236.c	(revision 0)
***************
*** 0 ****
--- 1,22 ----
+ struct X { int i; };
+ 
+ int __attribute__((noinline))
+ foo (struct X *p, int *q, int a, int b)
+ {
+   struct X x, y;
+   if (a)
+     p = &x;
+   if (b)
+     q = &x.i;
+   else
+     q = &y.i;
+   *q = 1;
+   return p->i; 
+ }
+ extern void abort (void);
+ int main()
+ {
+   if (foo((void *)0, (void *)0, 1, 1) != 1)
+     abort ();
+   return 0;
+ }