Re: [patch] Fix PR tree-optimization/70884

[Martin Jambor <mjambor@suse.cz>]

Hi,

On Fri, May 13, 2016 at 01:01:50PM +0200, Eric Botcazou wrote:
> > Hmm, the patch looks obvious if it was the intent to allow constant
> > pool replacements
> > _not_ only when the whole constant pool entry may go away.  But I
> > think the intent was
> > to not do this otherwise it will generate worse code by forcing all
> > loads from the constant pool to appear at
> > function start.
> 
> Do you mean when the whole constant pool entry is scalarized as opposed to 
> partially scalarized?
> 
> > So - the "real" issue may be a missing
> > should_scalarize_away_bitmap/cannot_scalarize_away_bitmap
> > check somewhere.
> 
> This seems to work:
> 
> Index: tree-sra.c
> ===================================================================
> --- tree-sra.c  (revision 236195)
> +++ tree-sra.c  (working copy)
> @@ -2680,6 +2680,10 @@ analyze_all_variable_accesses (void)
>    EXECUTE_IF_SET_IN_BITMAP (tmp, 0, i, bi)
>      {
>        tree var = candidate (i);
> +      if (constant_decl_p (var)
> +         && (!bitmap_bit_p (should_scalarize_away_bitmap, i)
> +             || bitmap_bit_p (cannot_scalarize_away_bitmap, i)))
> +       continue;
>        struct access *access;
>  
>        access = sort_and_splice_var_accesses (var);
> 
> but I have no idea whether this is correct or not.

This would skip creation of access trees for the variables without
disqualifying them and while it may "work," it is certainly ugly,
causing lookups to traverse uninitialized structures.

> 
> Martin, are we sure to disable scalarization of constant_decl_p variables not 
> covered by initialize_constant_pool_replacements that way?

Overall, I think that removal of bitmap tests in
initialize_constant_pool_replacements is certainly correct, if SRA
decides to create replacements for a constant pool decl, it has to
initialize them.  Whether we want to perform such scalarization is
another matter.

SRA creates replacement for all parts of an aggregate that is loaded
as a scalar multiple times.  If moving (multiple) scalar loads from
the constant pool to the beginning of the function is a bad idea, then
I would propose the fix below which disables that heuristics for them.
Effectively, the patch prevents late-SRA from doing anything for both
testcases (PR 70884 and PR 70919).  I have started a bootstrap and
testing on x86_64 and i686 only a few moments ago but it would be
great if someone also tried on an architecture for which the
constant-pool SRA enhancement was intended, just to be sure.

Thanks,

Martin


2016-05-13  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/70884
	* tree-sra.c (initialize_constant_pool_replacements): Do not check
	should_scalarize_away_bitmap and cannot_scalarize_away_bitmap bits.
	(sort_and_splice_var_accesses): Do not consider multiple scalar reads
	of constant pool data as a reason for scalarization.

testsuite/
	* gcc.dg/tree-ssa/pr70919.c: New test.
---
 gcc/testsuite/gcc.dg/tree-ssa/pr70919.c | 46 ++++++++++++++++++++++++++++
 gcc/tree-sra.c                          | 54 ++++++++++++++++-----------------
 2 files changed, 73 insertions(+), 27 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/pr70919.c

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr70919.c b/gcc/testsuite/gcc.dg/tree-ssa/pr70919.c
new file mode 100644
index 0000000..bed0ab3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr70919.c
@@ -0,0 +1,46 @@
+/* { dg-do run } */
+/* { dg-options "-O" } */
+
+#pragma pack(1)
+struct S0
+{
+  int f0:24;
+};
+
+struct S1
+{
+  int f1;
+} a;
+
+int b, c;
+
+char
+fn1 (struct S1 p1)
+{
+  return 0;
+}
+
+int
+main ()
+{
+  c = fn1 (a);
+  if (b)
+    {
+      struct S0 f[3][9] =
+	{ { { 0 }, { 0 }, { 1 }, { 1 }, { 0 }, { 0 }, { 0 }, { 1 }, { 1 } },
+	  { { 0 }, { 0 }, { 1 }, { 1 }, { 0 }, { 0 }, { 0 }, { 1 }, { 1 } },
+	  { { 0 }, { 0 }, { 1 }, { 1 }, { 0 }, { 0 }, { 0 }, { 1 }, { 1 } }
+	};
+      b = f[1][8].f0;
+    }
+  struct S0 g[3][9] =
+	{ { { 0 }, { 0 }, { 1 }, { 1 }, { 0 }, { 0 }, { 0 }, { 1 }, { 1 } },
+	  { { 0 }, { 0 }, { 1 }, { 1 }, { 0 }, { 0 }, { 0 }, { 1 }, { 1 } },
+	  { { 0 }, { 0 }, { 1 }, { 1 }, { 0 }, { 0 }, { 0 }, { 1 }, { 1 } }
+	};
+
+  if (g[1][8].f0 != 1)
+    __builtin_abort ();
+
+  return 0;
+}
diff --git a/gcc/tree-sra.c b/gcc/tree-sra.c
index 936d3a6..7c0e90d 100644
--- a/gcc/tree-sra.c
+++ b/gcc/tree-sra.c
@@ -2074,7 +2074,8 @@ sort_and_splice_var_accesses (tree var)
       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_scalar_reads || total_scalarization;
+      access->grp_hint = total_scalarization
+	|| (multiple_scalar_reads && !constant_decl_p (var));
       access->grp_total_scalarization = total_scalarization;
       access->grp_partial_lhs = grp_partial_lhs;
       access->grp_unscalarizable_region = unscalarizable_region;
@@ -3559,32 +3560,31 @@ initialize_constant_pool_replacements (void)
   unsigned i;
 
   EXECUTE_IF_SET_IN_BITMAP (candidate_bitmap, 0, i, bi)
-    if (bitmap_bit_p (should_scalarize_away_bitmap, i)
-	&& !bitmap_bit_p (cannot_scalarize_away_bitmap, i))
-      {
-	tree var = candidate (i);
-	if (!constant_decl_p (var))
-	  continue;
-	vec<access_p> *access_vec = get_base_access_vector (var);
-	if (!access_vec)
-	  continue;
-	for (unsigned i = 0; i < access_vec->length (); i++)
-	  {
-	    struct access *access = (*access_vec)[i];
-	    if (!access->replacement_decl)
-	      continue;
-	    gassign *stmt = gimple_build_assign (
-	      get_access_replacement (access), unshare_expr (access->expr));
-	    if (dump_file && (dump_flags & TDF_DETAILS))
-	      {
-		fprintf (dump_file, "Generating constant initializer: ");
-		print_gimple_stmt (dump_file, stmt, 0, 1);
-		fprintf (dump_file, "\n");
-	      }
-	    gsi_insert_after (&gsi, stmt, GSI_NEW_STMT);
-	    update_stmt (stmt);
-	  }
-      }
+    {
+      tree var = candidate (i);
+      if (!constant_decl_p (var))
+	continue;
+      vec<access_p> *access_vec = get_base_access_vector (var);
+      if (!access_vec)
+	continue;
+      for (unsigned i = 0; i < access_vec->length (); i++)
+	{
+	  struct access *access = (*access_vec)[i];
+	  if (!access->replacement_decl)
+	    continue;
+	  gassign *stmt
+	    = gimple_build_assign (get_access_replacement (access),
+				   unshare_expr (access->expr));
+	  if (dump_file && (dump_flags & TDF_DETAILS))
+	    {
+	      fprintf (dump_file, "Generating constant initializer: ");
+	      print_gimple_stmt (dump_file, stmt, 0, 1);
+	      fprintf (dump_file, "\n");
+	    }
+	  gsi_insert_after (&gsi, stmt, GSI_NEW_STMT);
+	  update_stmt (stmt);
+	}
+    }
 
   seq = gsi_seq (gsi);
   if (seq)
-- 
2.8.2