[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.

On Mon, Jun 28, 2010 at 4:17 PM, Michael Matz <matz@suse.de> wrote:
> 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?

Ok.

Thanks,
Richard.

>
> 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
>