diff mbox

[rfa] Fix PR44592: wrong code

Message ID Pine.LNX.4.64.1006251746400.18619@wotan.suse.de
State New
Headers show

Commit Message

Michael Matz June 25, 2010, 4:03 p.m. UTC 
Hello,

For callers of gimplify_and_update_call_from_tree() that possibly traverse 
the VOP chains we have to make the new sequence as a whole have the same 
effects as the original statements (from a VOP perspective), i.e. 
attaching the old VOPs to the new sequence.  There was a latent ommission 
uncovered by my more general use of the above function.  It involved 
transforming a call without LHS but VOPs (e.g. memcpy) into stores.

Hence, we track the last store of the new sequence, and possibly attach 
the VOPs of the original statement to that one.

Fixed the testcase, regstrapping on x86_64-linux in progress.  Okay for 
trunk and 4.5 after some time?


Ciao,
Michael.

Comments

Michael Matz June 25, 2010, 4:11 p.m. UTC | #1 
Hi,

On Fri, 25 Jun 2010, Michael Matz wrote:

> For callers of gimplify_and_update_call_from_tree() that possibly traverse 
> the VOP chains we have to make the new sequence as a whole have the same 
> effects as the original statements (from a VOP perspective), i.e. 
> attaching the old VOPs to the new sequence.  There was a latent ommission 
> uncovered by my more general use of the above function.  It involved 
> transforming a call without LHS but VOPs (e.g. memcpy) into stores.
> 
> Hence, we track the last store of the new sequence, and possibly attach 
> the VOPs of the original statement to that one.

Actually, thinking about this, to be really conservative we must expect 
multiple stores in the sequence, which still would be left in a broken 
state.  Luckily it's a linear sequence, so updating the VDEF chain therein 
is easy (inventing new SSA names for the non-last VDEFs).  I'm going to 
rework the patch towards that.


Ciao,
Michael.
diff mbox

Patch

Index: gimple-fold.c
===================================================================
--- gimple-fold.c	(revision 161389)
+++ gimple-fold.c	(working copy)
@@ -1065,6 +1065,7 @@  gimplify_and_update_call_from_tree (gimp
   gimple_seq stmts = gimple_seq_alloc();
   struct gimplify_ctx gctx;
   gimple last = NULL;
+  gimple laststore = NULL;
 
   stmt = gsi_stmt (*si_p);
 
@@ -1096,12 +1097,28 @@  gimplify_and_update_call_from_tree (gimp
       find_new_referenced_vars (new_stmt);
       mark_symbols_for_renaming (new_stmt);
       last = new_stmt;
+      if (gimple_assign_single_p (last)
+	  && !is_gimple_reg (gimple_assign_lhs (last)))
+	laststore = last;
     }
 
   if (lhs == NULL_TREE)
     {
-      unlink_stmt_vdef (stmt);
-      release_defs (stmt);
+      /* If we replace a call without LHS that has a VDEF and our new
+         sequence ends with a store we must make that store have the same
+	 vdef in order not to break the sequencing.  This can happen
+	 for instance when folding memcpy calls into assignments.  */
+      if (gimple_vdef (stmt) && laststore)
+	{
+	  gimple_set_vuse (laststore, gimple_vuse (stmt));
+	  gimple_set_vdef (laststore, gimple_vdef (stmt));
+	  move_ssa_defining_stmt_for_defs (laststore, stmt);
+	}
+      else
+	{
+	  unlink_stmt_vdef (stmt);
+	  release_defs (stmt);
+	}
       new_stmt = last;
     }
   else
Index: testsuite/gfortran.dg/pr44592.f90
===================================================================
--- testsuite/gfortran.dg/pr44592.f90	(revision 0)
+++ testsuite/gfortran.dg/pr44592.f90	(revision 0)
@@ -0,0 +1,20 @@ 
+! { dg-do run }
+! { dg-options "-O3" }
+! From forall_12.f90
+! Fails with loop reversal at -O3
+!
+  character(len=1) :: b(4) = (/"1","2","3","4"/), c(4)
+  c = b
+  i = 1
+  ! This statement must be here for the abort below
+  b(1:3)(i:i) = b(2:4)(i:i)
+
+  b = c
+  b(4:2:-1)(i:i) = b(3:1:-1)(i:i)
+
+  ! This fails.  If the condition is printed, the result is F F F F
+  if (any (b .ne. (/"1","1","2","3"/))) i = 2
+  print *, b
+  print *, b .ne. (/"1","1","2","3"/)
+  if (i == 2) call abort
+end