Re: [rfa] Fix PR44592: wrong code

[Richard Guenther <richard dot guenther at gmail dot com>]

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
>