Make SRA create statements with the correct alias type

Hi,

the patch below is inspired by PR 57297 (the most relevant comments
are #4 and #5).  The problem is that currently SRA creates memory
loads and stores with alias type of whatever happens to be in
access->base.  However, at least when using placement or some nasty
type-casting, it is possible that the same aggregate, represented by
the same access structure, is accessed using different alias types in
one function.  This might lead to bogus memory access reordering, at
least in theory.  This patch therefore makes sure all SRA created
accesses have the same alias type as the load/store they originated
from.

Because load_assign_lhs_subreplacements did not look like it could
accept one more parameter, I encapsulated all of them in a structure.
I wrote this patch in December, I admit I don't remember what the new
testcase aims for, but I assume I added it for a reason :-)

Bootstrapped and tested on x86_64-linux.  OK for trunk?

Thanks,

Martin


2014-04-24  Martin Jambor  <mjambor@suse.cz>

	* tree-sra.c (sra_modify_expr): Generate new memory accesses with
	same alias type as the original statement.
	(subreplacement_assignment_data): New type.
	(handle_unscalarized_data_in_subtree): New type of parameter,
	generate new memory accesses with same alias type as the original
	statement.
	(load_assign_lhs_subreplacements): Likewise.
	(sra_modify_constructor_assign): Generate new memory accesses with
	same alias type as the original statement.

testsuite/
	* gcc.dg/tree-ssa/sra-14.c: New test.

On Fri, 25 Apr 2014, Martin Jambor wrote:

> Hi,
> 
> the patch below is inspired by PR 57297 (the most relevant comments
> are #4 and #5).  The problem is that currently SRA creates memory
> loads and stores with alias type of whatever happens to be in
> access->base.  However, at least when using placement or some nasty
> type-casting, it is possible that the same aggregate, represented by
> the same access structure, is accessed using different alias types in
> one function.  This might lead to bogus memory access reordering, at
> least in theory.  This patch therefore makes sure all SRA created
> accesses have the same alias type as the load/store they originated
> from.
> 
> Because load_assign_lhs_subreplacements did not look like it could
> accept one more parameter, I encapsulated all of them in a structure.
> I wrote this patch in December, I admit I don't remember what the new
> testcase aims for, but I assume I added it for a reason :-)
> 
> Bootstrapped and tested on x86_64-linux.  OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
> 
> Martin
> 
> 
> 2014-04-24  Martin Jambor  <mjambor@suse.cz>
> 
> 	* tree-sra.c (sra_modify_expr): Generate new memory accesses with
> 	same alias type as the original statement.
> 	(subreplacement_assignment_data): New type.
> 	(handle_unscalarized_data_in_subtree): New type of parameter,
> 	generate new memory accesses with same alias type as the original
> 	statement.
> 	(load_assign_lhs_subreplacements): Likewise.
> 	(sra_modify_constructor_assign): Generate new memory accesses with
> 	same alias type as the original statement.
> 
> testsuite/
> 	* gcc.dg/tree-ssa/sra-14.c: New test.
> 
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/sra-14.c b/gcc/testsuite/gcc.dg/tree-ssa/sra-14.c
> new file mode 100644
> index 0000000..6cbc0b4
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/sra-14.c
> @@ -0,0 +1,70 @@
> +/* { dg-do run } */
> +/* { dg-options "-O1" } */
> +
> +struct S
> +{
> +  int i, j;
> +};
> +
> +struct Z
> +{
> +  struct S d, s;
> +};
> +
> +struct S __attribute__ ((noinline, noclone))
> +get_s (void)
> +{
> +  struct S s;
> +  s.i = 5;
> +  s.j = 6;
> +
> +  return s;
> +}
> +
> +struct S __attribute__ ((noinline, noclone))
> +get_d (void)
> +{
> +  struct S d;
> +  d.i = 0;
> +  d.j = 0;
> +
> +  return d;
> +}
> +
> +int __attribute__ ((noinline, noclone))
> +get_c (void)
> +{
> +  return 1;
> +}
> +
> +int __attribute__ ((noinline, noclone))
> +my_nop (int i)
> +{
> +  return i;
> +}
> +
> +int __attribute__ ((noinline, noclone))
> +foo (void)
> +{
> +  struct Z z;
> +  int i, c = get_c ();
> +
> +  z.d = get_d ();
> +  z.s = get_s ();
> +
> +  for (i = 0; i < c; i++)
> +    {
> +      z.s.i = my_nop (z.s.i);
> +      z.s.j = my_nop (z.s.j);
> +    }
> +
> +  return z.s.i + z.s.j;
> +}
> +
> +int main (int argc, char *argv[])
> +{
> +  if (foo () != 11)
> +    __builtin_abort ();
> +  return 0;
> +}
> +
> diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
> index 49bbee3..4a24e6a 100644
> --- a/gcc/tree-sra.c
> +++ b/gcc/tree-sra.c
> @@ -2769,7 +2769,7 @@ sra_modify_expr (tree *expr, gimple_stmt_iterator *gsi, bool write)
>  {
>    location_t loc;
>    struct access *access;
> -  tree type, bfr;
> +  tree type, bfr, orig_expr;
>  
>    if (TREE_CODE (*expr) == BIT_FIELD_REF)
>      {
> @@ -2785,6 +2785,7 @@ sra_modify_expr (tree *expr, gimple_stmt_iterator *gsi, bool write)
>    if (!access)
>      return false;
>    type = TREE_TYPE (*expr);
> +  orig_expr = *expr;
>  
>    loc = gimple_location (gsi_stmt (*gsi));
>    gimple_stmt_iterator alt_gsi = gsi_none ();
> @@ -2811,8 +2812,7 @@ sra_modify_expr (tree *expr, gimple_stmt_iterator *gsi, bool write)
>  	{
>  	  tree ref;
>  
> -	  ref = build_ref_for_model (loc, access->base, access->offset, access,
> -				     NULL, false);
> +	  ref = build_ref_for_model (loc, orig_expr, 0, access, NULL, false);
>  
>  	  if (write)
>  	    {
> @@ -2863,7 +2863,7 @@ sra_modify_expr (tree *expr, gimple_stmt_iterator *gsi, bool write)
>        else
>  	start_offset = chunk_size = 0;
>  
> -      generate_subtree_copies (access->first_child, access->base, 0,
> +      generate_subtree_copies (access->first_child, orig_expr, access->offset,
>  			       start_offset, chunk_size, gsi, write, write,
>  			       loc);
>      }
> @@ -2877,53 +2877,70 @@ enum unscalarized_data_handling { SRA_UDH_NONE,  /* Nothing done so far. */
>  				  SRA_UDH_RIGHT, /* Data flushed to the RHS. */
>  				  SRA_UDH_LEFT }; /* Data flushed to the LHS. */
>  
> +struct subreplacement_assignment_data
> +{
> +  /* Offset of the access representing the lhs of the assignment.  */
> +  HOST_WIDE_INT left_offset;
> +
> +  /* LHS and RHS of the original assignment.  */
> +  tree assignment_lhs, assignment_rhs;
> +
> +  /* Access representing the rhs of the whole assignment.  */
> +  struct access *top_racc;
> +
> +  /* Stmt iterator used for statement insertions after the original assignment.
> +   It points to the main GSI used to traverse a BB during function body
> +   modification.  */
> +  gimple_stmt_iterator *new_gsi;
> +
> +  /* Stmt iterator used for statement insertions before the original
> +   assignment.  Keeps on pointing to the original statement.  */
> +  gimple_stmt_iterator old_gsi;
> +
> +  /* Location of the assignment.   */
> +  location_t loc;
> +
> +  /* Keeps the information whether we have needed to refresh replacements of
> +   the LHS and from which side of the assignments this takes place.  */
> +  enum unscalarized_data_handling refreshed;
> +};
> +
>  /* Store all replacements in the access tree rooted in TOP_RACC either to their
>     base aggregate if there are unscalarized data or directly to LHS of the
>     statement that is pointed to by GSI otherwise.  */
>  
> -static enum unscalarized_data_handling
> -handle_unscalarized_data_in_subtree (struct access *top_racc,
> -				     gimple_stmt_iterator *gsi)
> +static void
> +handle_unscalarized_data_in_subtree (struct subreplacement_assignment_data *sad)
>  {
> -  if (top_racc->grp_unscalarized_data)
> +  tree src;
> +  if (sad->top_racc->grp_unscalarized_data)
>      {
> -      generate_subtree_copies (top_racc->first_child, top_racc->base, 0, 0, 0,
> -			       gsi, false, false,
> -			       gimple_location (gsi_stmt (*gsi)));
> -      return SRA_UDH_RIGHT;
> +      src = sad->assignment_rhs;
> +      sad->refreshed = SRA_UDH_RIGHT;
>      }
>    else
>      {
> -      tree lhs = gimple_assign_lhs (gsi_stmt (*gsi));
> -      generate_subtree_copies (top_racc->first_child, lhs, top_racc->offset,
> -			       0, 0, gsi, false, false,
> -			       gimple_location (gsi_stmt (*gsi)));
> -      return SRA_UDH_LEFT;
> +      src = sad->assignment_lhs;
> +      sad->refreshed = SRA_UDH_LEFT;
>      }
> +  generate_subtree_copies (sad->top_racc->first_child, src,
> +			   sad->top_racc->offset, 0, 0,
> +			   &sad->old_gsi, false, false, sad->loc);
>  }
>  
> -
>  /* Try to generate statements to load all sub-replacements in an access subtree
> -   formed by children of LACC from scalar replacements in the TOP_RACC subtree.
> -   If that is not possible, refresh the TOP_RACC base aggregate and load the
> -   accesses from it.  LEFT_OFFSET is the offset of the left whole subtree being
> -   copied. NEW_GSI is stmt iterator used for statement insertions after the
> -   original assignment, OLD_GSI is used to insert statements before the
> -   assignment.  *REFRESHED keeps the information whether we have needed to
> -   refresh replacements of the LHS and from which side of the assignments this
> -   takes place.  */
> +   formed by children of LACC from scalar replacements in the SAD->top_racc
> +   subtree.  If that is not possible, refresh the SAD->top_racc base aggregate
> +   and load the accesses from it.  */
>  
>  static void
> -load_assign_lhs_subreplacements (struct access *lacc, struct access *top_racc,
> -				 HOST_WIDE_INT left_offset,
> -				 gimple_stmt_iterator *old_gsi,
> -				 gimple_stmt_iterator *new_gsi,
> -				 enum unscalarized_data_handling *refreshed)
> +load_assign_lhs_subreplacements (struct access *lacc,
> +				 struct subreplacement_assignment_data *sad)
>  {
> -  location_t loc = gimple_location (gsi_stmt (*old_gsi));
>    for (lacc = lacc->first_child; lacc; lacc = lacc->next_sibling)
>      {
> -      HOST_WIDE_INT offset = lacc->offset - left_offset + top_racc->offset;
> +      HOST_WIDE_INT offset;
> +      offset = lacc->offset - sad->left_offset + sad->top_racc->offset;
>  
>        if (lacc->grp_to_be_replaced)
>  	{
> @@ -2931,53 +2948,57 @@ load_assign_lhs_subreplacements (struct access *lacc, struct access *top_racc,
>  	  gimple stmt;
>  	  tree rhs;
>  
> -	  racc = find_access_in_subtree (top_racc, offset, lacc->size);
> +	  racc = find_access_in_subtree (sad->top_racc, offset, lacc->size);
>  	  if (racc && racc->grp_to_be_replaced)
>  	    {
>  	      rhs = get_access_replacement (racc);
>  	      if (!useless_type_conversion_p (lacc->type, racc->type))
> -		rhs = fold_build1_loc (loc, VIEW_CONVERT_EXPR, lacc->type, rhs);
> +		rhs = fold_build1_loc (sad->loc, VIEW_CONVERT_EXPR,
> +				       lacc->type, rhs);
>  
>  	      if (racc->grp_partial_lhs && lacc->grp_partial_lhs)
> -		rhs = force_gimple_operand_gsi (old_gsi, rhs, true, NULL_TREE,
> -						true, GSI_SAME_STMT);
> +		rhs = force_gimple_operand_gsi (&sad->old_gsi, rhs, true,
> +						NULL_TREE, true, GSI_SAME_STMT);
>  	    }
>  	  else
>  	    {
>  	      /* No suitable access on the right hand side, need to load from
>  		 the aggregate.  See if we have to update it first... */
> -	      if (*refreshed == SRA_UDH_NONE)
> -		*refreshed = handle_unscalarized_data_in_subtree (top_racc,
> -								  old_gsi);
> +	      if (sad->refreshed == SRA_UDH_NONE)
> +		handle_unscalarized_data_in_subtree (sad);
>  
> -	      if (*refreshed == SRA_UDH_LEFT)
> -		rhs = build_ref_for_model (loc, lacc->base, lacc->offset, lacc,
> -					    new_gsi, true);
> +	      if (sad->refreshed == SRA_UDH_LEFT)
> +		rhs = build_ref_for_model (sad->loc, sad->assignment_lhs,
> +					   lacc->offset - sad->left_offset,
> +					   lacc, sad->new_gsi, true);
>  	      else
> -		rhs = build_ref_for_model (loc, top_racc->base, offset, lacc,
> -					    new_gsi, true);
> +		rhs = build_ref_for_model (sad->loc, sad->assignment_rhs,
> +					   lacc->offset - sad->left_offset,
> +					   lacc, sad->new_gsi, true);
>  	      if (lacc->grp_partial_lhs)
> -		rhs = force_gimple_operand_gsi (new_gsi, rhs, true, NULL_TREE,
> +		rhs = force_gimple_operand_gsi (sad->new_gsi,
> +						rhs, true, NULL_TREE,
>  						false, GSI_NEW_STMT);
>  	    }
>  
>  	  stmt = gimple_build_assign (get_access_replacement (lacc), rhs);
> -	  gsi_insert_after (new_gsi, stmt, GSI_NEW_STMT);
> -	  gimple_set_location (stmt, loc);
> +	  gsi_insert_after (sad->new_gsi, stmt, GSI_NEW_STMT);
> +	  gimple_set_location (stmt, sad->loc);
>  	  update_stmt (stmt);
>  	  sra_stats.subreplacements++;
>  	}
>        else
>  	{
> -	  if (*refreshed == SRA_UDH_NONE
> +	  if (sad->refreshed == SRA_UDH_NONE
>  	      && lacc->grp_read && !lacc->grp_covered)
> -	    *refreshed = handle_unscalarized_data_in_subtree (top_racc,
> -							      old_gsi);
> +	    handle_unscalarized_data_in_subtree (sad);
> +
>  	  if (lacc && lacc->grp_to_be_debug_replaced)
>  	    {
>  	      gimple ds;
>  	      tree drhs;
> -	      struct access *racc = find_access_in_subtree (top_racc, offset,
> +	      struct access *racc = find_access_in_subtree (sad->top_racc,
> +							    offset,
>  							    lacc->size);
>  
>  	      if (racc && racc->grp_to_be_replaced)
> @@ -2987,27 +3008,26 @@ load_assign_lhs_subreplacements (struct access *lacc, struct access *top_racc,
>  		  else
>  		    drhs = NULL;
>  		}
> -	      else if (*refreshed == SRA_UDH_LEFT)
> -		drhs = build_debug_ref_for_model (loc, lacc->base, lacc->offset,
> -						  lacc);
> -	      else if (*refreshed == SRA_UDH_RIGHT)
> -		drhs = build_debug_ref_for_model (loc, top_racc->base, offset,
> -						  lacc);
> +	      else if (sad->refreshed == SRA_UDH_LEFT)
> +		drhs = build_debug_ref_for_model (sad->loc, lacc->base,
> +						  lacc->offset, lacc);
> +	      else if (sad->refreshed == SRA_UDH_RIGHT)
> +		drhs = build_debug_ref_for_model (sad->loc, sad->top_racc->base,
> +						  offset, lacc);
>  	      else
>  		drhs = NULL_TREE;
>  	      if (drhs
>  		  && !useless_type_conversion_p (lacc->type, TREE_TYPE (drhs)))
> -		drhs = fold_build1_loc (loc, VIEW_CONVERT_EXPR,
> +		drhs = fold_build1_loc (sad->loc, VIEW_CONVERT_EXPR,
>  					lacc->type, drhs);
>  	      ds = gimple_build_debug_bind (get_access_replacement (lacc),
> -					    drhs, gsi_stmt (*old_gsi));
> -	      gsi_insert_after (new_gsi, ds, GSI_NEW_STMT);
> +					    drhs, gsi_stmt (sad->old_gsi));
> +	      gsi_insert_after (sad->new_gsi, ds, GSI_NEW_STMT);
>  	    }
>  	}
>  
>        if (lacc->first_child)
> -	load_assign_lhs_subreplacements (lacc, top_racc, left_offset,
> -					 old_gsi, new_gsi, refreshed);
> +	load_assign_lhs_subreplacements (lacc, sad);
>      }
>  }
>  
> @@ -3053,7 +3073,7 @@ sra_modify_constructor_assign (gimple *stmt, gimple_stmt_iterator *gsi)
>        /* I have never seen this code path trigger but if it can happen the
>  	 following should handle it gracefully.  */
>        if (access_has_children_p (acc))
> -	generate_subtree_copies (acc->first_child, acc->base, 0, 0, 0, gsi,
> +	generate_subtree_copies (acc->first_child, lhs, acc->offset, 0, 0, gsi,
>  				 true, true, loc);
>        return SRA_AM_MODIFIED;
>      }
> @@ -3261,7 +3281,7 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator *gsi)
>        || stmt_ends_bb_p (*stmt))
>      {
>        if (access_has_children_p (racc))
> -	generate_subtree_copies (racc->first_child, racc->base, 0, 0, 0,
> +	generate_subtree_copies (racc->first_child, rhs, racc->offset, 0, 0,
>  				 gsi, false, false, loc);
>        if (access_has_children_p (lacc))
>  	{
> @@ -3271,7 +3291,7 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator *gsi)
>  	      alt_gsi = gsi_start_edge (single_non_eh_succ (gsi_bb (*gsi)));
>  	      gsi = &alt_gsi;
>  	    }
> -	  generate_subtree_copies (lacc->first_child, lacc->base, 0, 0, 0,
> +	  generate_subtree_copies (lacc->first_child, lhs, lacc->offset, 0, 0,
>  				   gsi, true, true, loc);
>  	}
>        sra_stats.separate_lhs_rhs_handling++;
> @@ -3301,21 +3321,26 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator *gsi)
>  	  && !lacc->grp_unscalarizable_region
>  	  && !racc->grp_unscalarizable_region)
>  	{
> -	  gimple_stmt_iterator orig_gsi = *gsi;
> -	  enum unscalarized_data_handling refreshed;
> +	  struct subreplacement_assignment_data sad;
> +
> +	  sad.left_offset = lacc->offset;
> +	  sad.assignment_lhs = lhs;
> +	  sad.assignment_rhs = rhs;
> +	  sad.top_racc = racc;
> +	  sad.old_gsi = *gsi;
> +	  sad.new_gsi = gsi;
> +	  sad.loc = gimple_location (*stmt);
> +	  sad.refreshed = SRA_UDH_NONE;
>  
>  	  if (lacc->grp_read && !lacc->grp_covered)
> -	    refreshed = handle_unscalarized_data_in_subtree (racc, gsi);
> -	  else
> -	    refreshed = SRA_UDH_NONE;
> +	    handle_unscalarized_data_in_subtree (&sad);
>  
> -	  load_assign_lhs_subreplacements (lacc, racc, lacc->offset,
> -					   &orig_gsi, gsi, &refreshed);
> -	  if (refreshed != SRA_UDH_RIGHT)
> +	  load_assign_lhs_subreplacements (lacc, &sad);
> +	  if (sad.refreshed != SRA_UDH_RIGHT)
>  	    {
>  	      gsi_next (gsi);
>  	      unlink_stmt_vdef (*stmt);
> -	      gsi_remove (&orig_gsi, true);
> +	      gsi_remove (&sad.old_gsi, true);
>  	      release_defs (*stmt);
>  	      sra_stats.deleted++;
>  	      return SRA_AM_REMOVED;
> @@ -3344,7 +3369,7 @@ sra_modify_assign (gimple *stmt, gimple_stmt_iterator *gsi)
>  	  /* Restore the aggregate RHS from its components so the
>  	     prevailing aggregate copy does the right thing.  */
>  	  if (access_has_children_p (racc))
> -	    generate_subtree_copies (racc->first_child, racc->base, 0, 0, 0,
> +	    generate_subtree_copies (racc->first_child, rhs, racc->offset, 0, 0,
>  				     gsi, false, false, loc);
>  	  /* Re-load the components of the aggregate copy destination.
>  	     But use the RHS aggregate to load from to expose more
> 
> 
>