This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [rfa] Fix PR44592: wrong code
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.
Like this. Regstrapped on x86_64-linux (all langs+Ada), no regressions.
Okay for trunk and 4.5 after some time?
Ciao,
Michael.
--
PR middle-end/44592
* gimple-fold.c (gimplify_and_update_call_from_tree): Maintain
proper VDEF chain for intermediate stores in the sequence.
testsuite/
PR middle-end/44592
* gfortran.dg/pr44592.f90: New test.
Index: gimple-fold.c
===================================================================
--- gimple-fold.c (revision 161389)
+++ gimple-fold.c (working copy)
@@ -1053,7 +1053,9 @@ fold_gimple_cond (gimple stmt)
is replaced. If the call is expected to produces a result, then it
is replaced by an assignment of the new RHS to the result variable.
If the result is to be ignored, then the call is replaced by a
- GIMPLE_NOP. */
+ GIMPLE_NOP. A proper VDEF chain is retained by making the first
+ VUSE and the last VDEF of the whole sequence be the same as the replaced
+ statement and using new SSA names for stores in between. */
void
gimplify_and_update_call_from_tree (gimple_stmt_iterator *si_p, tree expr)
@@ -1065,12 +1067,15 @@ gimplify_and_update_call_from_tree (gimp
gimple_seq stmts = gimple_seq_alloc();
struct gimplify_ctx gctx;
gimple last = NULL;
+ gimple laststore = NULL;
+ tree reaching_vuse;
stmt = gsi_stmt (*si_p);
gcc_assert (is_gimple_call (stmt));
lhs = gimple_call_lhs (stmt);
+ reaching_vuse = gimple_vuse (stmt);
push_gimplify_context (&gctx);
@@ -1095,13 +1100,47 @@ gimplify_and_update_call_from_tree (gimp
new_stmt = gsi_stmt (i);
find_new_referenced_vars (new_stmt);
mark_symbols_for_renaming (new_stmt);
+ /* If the new statement has a VUSE, update it with exact SSA name we
+ know will reach this one. */
+ if (gimple_vuse (new_stmt))
+ {
+ /* If we've also seen a previous store create a new VDEF for
+ the latter one, and make that the new reaching VUSE. */
+ if (laststore)
+ {
+ reaching_vuse = make_ssa_name (gimple_vop (cfun), laststore);
+ gimple_set_vdef (laststore, reaching_vuse);
+ update_stmt (laststore);
+ laststore = NULL;
+ }
+ gimple_set_vuse (new_stmt, reaching_vuse);
+ gimple_set_modified (new_stmt, true);
+ }
+ if (gimple_assign_single_p (new_stmt)
+ && !is_gimple_reg (gimple_assign_lhs (new_stmt)))
+ {
+ laststore = new_stmt;
+ }
last = new_stmt;
}
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_vdef (laststore, gimple_vdef (stmt));
+ move_ssa_defining_stmt_for_defs (laststore, stmt);
+ update_stmt (laststore);
+ }
+ else
+ {
+ unlink_stmt_vdef (stmt);
+ release_defs (stmt);
+ }
new_stmt = last;
}
else
@@ -1111,8 +1150,15 @@ gimplify_and_update_call_from_tree (gimp
gsi_insert_before (si_p, last, GSI_NEW_STMT);
gsi_next (si_p);
}
+ if (laststore)
+ {
+ reaching_vuse = make_ssa_name (gimple_vop (cfun), laststore);
+ gimple_set_vdef (laststore, reaching_vuse);
+ update_stmt (laststore);
+ laststore = NULL;
+ }
new_stmt = gimple_build_assign (lhs, tmp);
- gimple_set_vuse (new_stmt, gimple_vuse (stmt));
+ gimple_set_vuse (new_stmt, reaching_vuse);
gimple_set_vdef (new_stmt, gimple_vdef (stmt));
move_ssa_defining_stmt_for_defs (new_stmt, stmt);
}
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