diff mbox

[PR,45505] Fix SRA heuristics for single-field structures

Message ID 20110208141011.GC21798@virgil.arch.suse.de
State New
Headers show

Commit Message

Martin Jambor Feb. 8, 2011, 2:10 p.m. UTC
Hi,

the SRA heuristics sometimes does not work as intended for
single-field structures where the flags for the aggregate and the
scalar component get confused and we end up scalarizing even though we
should not.  No one would probably ever realize this if it did not
cause a difference in lines on which warnings are reported on

Comments

Richard Biener Feb. 8, 2011, 3:13 p.m. UTC | #1
On Tue, 8 Feb 2011, Martin Jambor wrote:

> Hi,
> 
> the SRA heuristics sometimes does not work as intended for
> single-field structures where the flags for the aggregate and the
> scalar component get confused and we end up scalarizing even though we
> should not.  No one would probably ever realize this if it did not
> cause a difference in lines on which warnings are reported on
> different architectures, known as PR 45505.  I'm sorry the description
> of the problem is so long but it really is a tiny devil in the
> details.
> 
> The problem is that the current code assumes that if a scalar access
> has its grp_read or grp_write set by sort_and_splice_var_accesses(),
> it means there was a direct scalar load or store to that part of the
> aggregate.  However, if there is a read of the aggregate and a write
> to the scalar component, that is not the same situation like having
> both scalar reads and writes (when we can hope to achieve copy
> propagation).  Slightly more complex confusions arise when an access
> for such a structure gets promoted to a scalar by a propagation across
> assignments - this is what happens in the PR.
> 
> The clean way of avoiding this is unfortunately to introduce yet
> another pair of read/write flags to the access structure, this time to

Eeek.

> mark accesses that were originally scalar instead of depending on the
> access->type of the representative.  With this information we can then
> make the heuristics work as intended.  We should also set grp_hint if
> there are multiple reads only if they are scalar.
> 
> The patch below does exactly that.  It only makes a difference for
> one-field structures and one element arrays and it makes SRA more
> conservative.  I believe the change is safe for stage 4 and the PR it
> fixes is a regression (though it is P2).
> 
> The patch has passed bootstrap and testsuite run on x86_64-linux, I'm
> currently running a testsuite on i686 too.  OK for trunk now if it
> passes there too?

Ok with ...

> Index: src/gcc/testsuite/gfortran.dg/pr25923.f90
> ===================================================================
> --- src.orig/gcc/testsuite/gfortran.dg/pr25923.f90
> +++ src/gcc/testsuite/gfortran.dg/pr25923.f90
> @@ -10,7 +10,7 @@ implicit none
>  
>  contains
>  
> -  function baz(arg) result(res) ! { dg-warning "res.yr' may be" "PR45505" { xfail ilp32 } }
> +  function baz(arg) result(res) ! { dg-bogus "res.yr' may be" "PR45505" }

remove the "PR45505" reference

>      type(bar), intent(in) :: arg
>      type(bar) :: res
>      logical, external:: some_func
> @@ -19,7 +19,7 @@ contains
>      else
>        res = arg
>      end if
> -  end function baz ! { dg-bogus "res.yr' may be" "PR45505" { xfail ilp32 } }
> +  end function baz ! { dg-warning "res.yr' may be" "PR45505" }

Likewise.

Thanks,
Richard.
diff mbox

Patch

different architectures, known as PR 45505.  I'm sorry the description
of the problem is so long but it really is a tiny devil in the
details.

The problem is that the current code assumes that if a scalar access
has its grp_read or grp_write set by sort_and_splice_var_accesses(),
it means there was a direct scalar load or store to that part of the
aggregate.  However, if there is a read of the aggregate and a write
to the scalar component, that is not the same situation like having
both scalar reads and writes (when we can hope to achieve copy
propagation).  Slightly more complex confusions arise when an access
for such a structure gets promoted to a scalar by a propagation across
assignments - this is what happens in the PR.

The clean way of avoiding this is unfortunately to introduce yet
another pair of read/write flags to the access structure, this time to
mark accesses that were originally scalar instead of depending on the
access->type of the representative.  With this information we can then
make the heuristics work as intended.  We should also set grp_hint if
there are multiple reads only if they are scalar.

The patch below does exactly that.  It only makes a difference for
one-field structures and one element arrays and it makes SRA more
conservative.  I believe the change is safe for stage 4 and the PR it
fixes is a regression (though it is P2).

The patch has passed bootstrap and testsuite run on x86_64-linux, I'm
currently running a testsuite on i686 too.  OK for trunk now if it
passes there too?

Thanks,

Martin


2011-02-07  Martin Jambor  <mjambor@suse.cz>

	PR middle-end/45505
	* tree-sra.c (struct access): New flags grp_scalar_read and
	grp_scalar_write.  Changed description of assignment read and write
	flags.
	(dump_access): Dump new flags, reorder all of them.
	(sort_and_splice_var_accesses): Set the new flag accordingly, use them
	to detect multiple scalar reads.
	(analyze_access_subtree): Use the new scalar read write flags instead
	of the old flags.  Adjusted comments.

	* testsuite/gfortran.dg/pr25923.f90: Swap dg-warning and dg-bogus,
	remove xfails.


Index: src/gcc/testsuite/gfortran.dg/pr25923.f90
===================================================================
--- src.orig/gcc/testsuite/gfortran.dg/pr25923.f90
+++ src/gcc/testsuite/gfortran.dg/pr25923.f90
@@ -10,7 +10,7 @@  implicit none
 
 contains
 
-  function baz(arg) result(res) ! { dg-warning "res.yr' may be" "PR45505" { xfail ilp32 } }
+  function baz(arg) result(res) ! { dg-bogus "res.yr' may be" "PR45505" }
     type(bar), intent(in) :: arg
     type(bar) :: res
     logical, external:: some_func
@@ -19,7 +19,7 @@  contains
     else
       res = arg
     end if
-  end function baz ! { dg-bogus "res.yr' may be" "PR45505" { xfail ilp32 } }
+  end function baz ! { dg-warning "res.yr' may be" "PR45505" }
 
 end module foo
 
Index: src/gcc/tree-sra.c
===================================================================
--- src.orig/gcc/tree-sra.c
+++ src/gcc/tree-sra.c
@@ -195,6 +195,14 @@  struct access
      statement?  This flag is propagated down the access tree.  */
   unsigned grp_assignment_write : 1;
 
+  /* Does this group contain a read access through a scalar type?  This flag is
+     not propagated in the access tree in any direction.  */
+  unsigned grp_scalar_read : 1;
+
+  /* Does this group contain a write access through a scalar type?  This flag
+     is not propagated in the access tree in any direction.  */
+  unsigned grp_scalar_write : 1;
+
   /* Other passes of the analysis use this bit to make function
      analyze_access_subtree create scalar replacements for this group if
      possible.  */
@@ -368,16 +376,18 @@  dump_access (FILE *f, struct access *acc
   fprintf (f, ", type = ");
   print_generic_expr (f, access->type, 0);
   if (grp)
-    fprintf (f, ", grp_write = %d, total_scalarization = %d, "
-	     "grp_read = %d, grp_hint = %d, grp_assignment_read = %d,"
-	     "grp_assignment_write = %d, grp_covered = %d, "
+    fprintf (f, ", total_scalarization = %d, grp_read = %d, grp_write = %d, "
+	     "grp_assignment_read = %d, grp_assignment_write = %d, "
+	     "grp_scalar_read = %d, grp_scalar_write = %d, "
+	     "grp_hint = %d, grp_covered = %d, "
 	     "grp_unscalarizable_region = %d, grp_unscalarized_data = %d, "
 	     "grp_partial_lhs = %d, grp_to_be_replaced = %d, "
 	     "grp_maybe_modified = %d, "
 	     "grp_not_necessarilly_dereferenced = %d\n",
-	     access->grp_write, access->total_scalarization,
-	     access->grp_read, access->grp_hint, access->grp_assignment_read,
-	     access->grp_assignment_write, access->grp_covered,
+	     access->total_scalarization, access->grp_read, access->grp_write,
+	     access->grp_assignment_read, access->grp_assignment_write,
+	     access->grp_scalar_read, access->grp_scalar_write,
+	     access->grp_hint, access->grp_covered,
 	     access->grp_unscalarizable_region, access->grp_unscalarized_data,
 	     access->grp_partial_lhs, access->grp_to_be_replaced,
 	     access->grp_maybe_modified,
@@ -1593,9 +1603,13 @@  sort_and_splice_var_accesses (tree var)
       struct access *access = VEC_index (access_p, access_vec, i);
       bool grp_write = access->write;
       bool grp_read = !access->write;
+      bool grp_scalar_write = access->write
+	&& is_gimple_reg_type (access->type);
+      bool grp_scalar_read = !access->write
+	&& is_gimple_reg_type (access->type);
       bool grp_assignment_read = access->grp_assignment_read;
       bool grp_assignment_write = access->grp_assignment_write;
-      bool multiple_reads = false;
+      bool multiple_scalar_reads = false;
       bool total_scalarization = access->total_scalarization;
       bool grp_partial_lhs = access->grp_partial_lhs;
       bool first_scalar = is_gimple_reg_type (access->type);
@@ -1620,13 +1634,21 @@  sort_and_splice_var_accesses (tree var)
 	  if (ac2->offset != access->offset || ac2->size != access->size)
 	    break;
 	  if (ac2->write)
-	    grp_write = true;
+	    {
+	      grp_write = true;
+	      grp_scalar_write = (grp_scalar_write
+				  || is_gimple_reg_type (ac2->type));
+	    }
 	  else
 	    {
-	      if (grp_read)
-		multiple_reads = true;
-	      else
-		grp_read = true;
+	      grp_read = true;
+	      if (is_gimple_reg_type (ac2->type))
+		{
+		  if (grp_scalar_read)
+		    multiple_scalar_reads = true;
+		  else
+		    grp_scalar_read = true;
+		}
 	    }
 	  grp_assignment_read |= ac2->grp_assignment_read;
 	  grp_assignment_write |= ac2->grp_assignment_write;
@@ -1648,9 +1670,11 @@  sort_and_splice_var_accesses (tree var)
       access->group_representative = access;
       access->grp_write = grp_write;
       access->grp_read = grp_read;
+      access->grp_scalar_read = grp_scalar_read;
+      access->grp_scalar_write = grp_scalar_write;
       access->grp_assignment_read = grp_assignment_read;
       access->grp_assignment_write = grp_assignment_write;
-      access->grp_hint = multiple_reads || total_scalarization;
+      access->grp_hint = multiple_scalar_reads || total_scalarization;
       access->grp_partial_lhs = grp_partial_lhs;
       access->grp_unscalarizable_region = unscalarizable_region;
       if (access->first_link)
@@ -1851,13 +1875,13 @@  enum mark_rw_status { SRA_MRRW_NOTHING,
    there is more than one direct read access) or according to the following
    table:
 
-   Access written to individually (once or more times)
+   Access written to through a scalar type (once or more times)
    |
-   |	Parent written to in an assignment statement
+   |	Written to in an assignment statement
    |	|
-   |	|	Access read individually _once_
+   |	|	Access read as scalar _once_
    |	|	|
-   |   	|	|	Parent read in an assignment statement
+   |   	|	|	Read in an assignment statement
    |	|	|	|
    |   	|	|	|	Scalarize	Comment
 -----------------------------------------------------------------------------
@@ -1888,8 +1912,6 @@  analyze_access_subtree (struct access *r
   HOST_WIDE_INT covered_to = root->offset;
   bool scalar = is_gimple_reg_type (root->type);
   bool hole = false, sth_created = false;
-  bool direct_read = root->grp_read;
-  bool direct_write = root->grp_write;
 
   if (root->grp_assignment_read)
     mark_read = SRA_MRRW_ASSIGN;
@@ -1938,8 +1960,8 @@  analyze_access_subtree (struct access *r
 
   if (allow_replacements && scalar && !root->first_child
       && (root->grp_hint
-	  || ((direct_write || root->grp_assignment_write)
-	      && (direct_read || root->grp_assignment_read))))
+	  || ((root->grp_scalar_read || root->grp_assignment_read)
+	      && (root->grp_scalar_write || root->grp_assignment_write))))
     {
       if (dump_file && (dump_flags & TDF_DETAILS))
 	{