This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH, PR 40493] Fix SRA miscompilation of binutils


Hi,

the patch below fixes two rather serious problems in the new SRA.

The  first one  is  that I  misremembered  the order  of arguments  of
BIT_FIELD_REF and was looking for the offset and size of the reference
at exactly the wrong places.

The second  is a slightly  more complex one.   sra_modify_assign() and
load_assign_lhs_subreplacements()  try hard  to  remove the  aggregate
assignment  if  possible  and  so  can  decide  to  flush  RHS  scalar
replacements  directly to  the  LHS  when it  knows  there's no  other
(unscalarized) data on the RHS.  That is all good and well except that
load_assign_lhs_subreplacements() always  looked to the  RHS aggregate
when it did  not find a RHS scalar replacement  corresponding to a LHS
scalar replacement.  However, that contained

This patch fixes  this changes a boolean variable  that keeps track of
whether scalars were flushed into one of the original aggregates to an
enum that also tells to which one and makes
load_assign_lhs_subreplacements use it to look at the correct place.

I have bootstrapped  and tested this on x86-64.   There was however an
acats new  failure which has something  to do with  timing and delays.
However, these happen  to me all the  time but go away when  I run the
tests again and so that's what I am doing right now.

So, is this OK provided that the acats failure does not reoccur?

Thanks,

Martin

2009-06-24  Martin Jambor  <mjambor@suse.cz>

	* tree-sra.c (sra_modify_expr): Correct BIT_FIELD_REF argument numbers.
	(enum unscalarized_data_handling): New type.
	(handle_unscalarized_data_in_subtree): Return what has been done.
	(load_assign_lhs_subreplacements): Handle left flushes differently.
	(sra_modify_assign): Use unscalarized_data_handling, simplified
	condition determining whether to remove the statement.

	* testsuite/gcc.c-torture/execute/pr40493.c: New test.

	
Index: mine/gcc/tree-sra.c
===================================================================
--- mine.orig/gcc/tree-sra.c
+++ mine/gcc/tree-sra.c
@@ -1907,8 +1907,8 @@ sra_modify_expr (tree *expr, gimple_stmt
 	  && host_integerp (TREE_OPERAND (bfr, 1), 1)
 	  && host_integerp (TREE_OPERAND (bfr, 2), 1))
 	{
-	  start_offset = tree_low_cst (TREE_OPERAND (bfr, 1), 1);
-	  chunk_size = tree_low_cst (TREE_OPERAND (bfr, 2), 1);
+	  chunk_size = tree_low_cst (TREE_OPERAND (bfr, 1), 1);
+	  start_offset = tree_low_cst (TREE_OPERAND (bfr, 2), 1);
 	}
       else
 	start_offset = chunk_size = 0;
@@ -1919,20 +1919,33 @@ sra_modify_expr (tree *expr, gimple_stmt
   return true;
 }
 
+/* Where scalar replacements of the RHS have been written to when a replacement
+   of a LHS of an assigments cannot be direclty loaded from a replacement of
+   the RHS. */
+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. */
+
 /* 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
    otherwise.  */
 
-static void
+static enum unscalarized_data_handling
 handle_unscalarized_data_in_subtree (struct access *top_racc, tree lhs,
 				     gimple_stmt_iterator *gsi)
 {
   if (top_racc->grp_unscalarized_data)
-    generate_subtree_copies (top_racc->first_child, top_racc->base, 0, 0, 0,
-			     gsi, false, false);
+    {
+      generate_subtree_copies (top_racc->first_child, top_racc->base, 0, 0, 0,
+			       gsi, false, false);
+      return SRA_UDH_RIGHT;
+    }
   else
-    generate_subtree_copies (top_racc->first_child, lhs, top_racc->offset,
-			     0, 0, gsi, false, false);
+    {
+      generate_subtree_copies (top_racc->first_child, lhs, top_racc->offset,
+			       0, 0, gsi, false, false);
+      return SRA_UDH_LEFT;
+    }
 }
 
 
@@ -1951,7 +1964,8 @@ load_assign_lhs_subreplacements (struct
 				 HOST_WIDE_INT right_offset,
 				 gimple_stmt_iterator *old_gsi,
 				 gimple_stmt_iterator *new_gsi,
-				 bool *refreshed, tree lhs)
+				 enum unscalarized_data_handling *refreshed,
+				 tree lhs)
 {
   do
     {
@@ -1975,18 +1989,20 @@ load_assign_lhs_subreplacements (struct
 
 	      /* No suitable access on the right hand side, need to load from
 		 the aggregate.  See if we have to update it first... */
-	      if (!*refreshed)
+	      if (*refreshed == SRA_UDH_NONE)
+		*refreshed = handle_unscalarized_data_in_subtree (top_racc,
+								  lhs, old_gsi);
+
+	      if (*refreshed == SRA_UDH_LEFT)
+		rhs = unshare_expr (lacc->expr);
+	      else
 		{
-		  gcc_assert (top_racc->first_child);
-		  handle_unscalarized_data_in_subtree (top_racc, lhs, old_gsi);
-		  *refreshed = true;
+		  rhs = unshare_expr (top_racc->base);
+		  repl_found = build_ref_for_offset (&rhs,
+						     TREE_TYPE (top_racc->base),
+						     offset, lacc->type, false);
+		  gcc_assert (repl_found);
 		}
-
-	      rhs = unshare_expr (top_racc->base);
-	      repl_found = build_ref_for_offset (&rhs,
-						 TREE_TYPE (top_racc->base),
-						 offset, lacc->type, false);
-	      gcc_assert (repl_found);
 	    }
 
 	  stmt = gimple_build_assign (get_access_replacement (lacc), rhs);
@@ -1994,11 +2010,10 @@ load_assign_lhs_subreplacements (struct
 	  update_stmt (stmt);
 	  sra_stats.subreplacements++;
 	}
-      else if (lacc->grp_read && !lacc->grp_covered && !*refreshed)
-	{
-	  handle_unscalarized_data_in_subtree (top_racc, lhs, old_gsi);
-	  *refreshed = true;
-	}
+      else if (*refreshed == SRA_UDH_NONE
+	       && lacc->grp_read && !lacc->grp_covered)
+	*refreshed = handle_unscalarized_data_in_subtree (top_racc, lhs,
+							  old_gsi);
 
       if (lacc->first_child)
 	load_assign_lhs_subreplacements (lacc->first_child, top_racc,
@@ -2204,20 +2219,17 @@ sra_modify_assign (gimple *stmt, gimple_
       if (access_has_children_p (lacc) && access_has_children_p (racc))
 	{
 	  gimple_stmt_iterator orig_gsi = *gsi;
-	  bool refreshed;
+	  enum unscalarized_data_handling refreshed;
 
 	  if (lacc->grp_read && !lacc->grp_covered)
-	    {
-	      handle_unscalarized_data_in_subtree (racc, lhs, gsi);
-	      refreshed = true;
-	    }
+	    refreshed = handle_unscalarized_data_in_subtree (racc, lhs, gsi);
 	  else
-	    refreshed = false;
+	    refreshed = SRA_UDH_NONE;
 
 	  load_assign_lhs_subreplacements (lacc->first_child, racc,
 					   lacc->offset, racc->offset,
 					   &orig_gsi, gsi, &refreshed, lhs);
-	  if (!refreshed || !racc->grp_unscalarized_data)
+	  if (refreshed != SRA_UDH_RIGHT)
 	    {
 	      if (*stmt == gsi_stmt (*gsi))
 		gsi_next (gsi);
Index: mine/gcc/testsuite/gcc.c-torture/execute/pr40493.c
===================================================================
--- /dev/null
+++ mine/gcc/testsuite/gcc.c-torture/execute/pr40493.c
@@ -0,0 +1,82 @@
+extern void abort (void);
+
+typedef union i386_operand_type
+{
+  struct
+    {
+      unsigned int reg8:1;
+      unsigned int reg16:1;
+      unsigned int reg32:1;
+      unsigned int reg64:1;
+      unsigned int floatreg:1;
+      unsigned int regmmx:1;
+      unsigned int regxmm:1;
+      unsigned int regymm:1;
+      unsigned int control:1;
+      unsigned int debug:1;
+      unsigned int test:1;
+      unsigned int sreg2:1;
+      unsigned int sreg3:1;
+      unsigned int imm1:1;
+      unsigned int imm8:1;
+      unsigned int imm8s:1;
+      unsigned int imm16:1;
+      unsigned int imm32:1;
+      unsigned int imm32s:1;
+      unsigned int imm64:1;
+      unsigned int disp8:1;
+      unsigned int disp16:1;
+      unsigned int disp32:1;
+      unsigned int disp32s:1;
+      unsigned int disp64:1;
+      unsigned int acc:1;
+      unsigned int floatacc:1;
+      unsigned int baseindex:1;
+      unsigned int inoutportreg:1;
+      unsigned int shiftcount:1;
+      unsigned int jumpabsolute:1;
+      unsigned int esseg:1;
+      unsigned int regmem:1;
+      unsigned int mem:1;
+      unsigned int byte:1;
+      unsigned int word:1;
+      unsigned int dword:1;
+      unsigned int fword:1;
+      unsigned int qword:1;
+      unsigned int tbyte:1;
+      unsigned int xmmword:1;
+      unsigned int ymmword:1;
+      unsigned int unspecified:1;
+      unsigned int anysize:1;
+    } bitfield;
+  unsigned int array[2];
+} i386_operand_type;
+
+unsigned int x00, x01, y00, y01;
+
+int main (int argc, char *argv[])
+{
+  i386_operand_type a,b,c,d;
+
+  a.bitfield.reg16 = 1;
+  a.bitfield.imm16 = 0;
+  a.array[1] = 22;
+
+  b = a;
+  x00 = b.array[0];
+  x01 = b.array[1];
+
+  c = b;
+  y00 = c.array[0];
+  y01 = c.array[1];
+
+  d = c;
+  if (d.bitfield.reg16 != 1)
+    abort();
+  if (d.bitfield.imm16 != 0)
+    abort();
+  if (d.array[1] != 22)
+    abort();
+
+  return 0;
+}


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]