From patchwork Mon Jun 28 14:17:26 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Matz X-Patchwork-Id: 57143 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 31348B6EEB for ; Tue, 29 Jun 2010 00:18:00 +1000 (EST) Received: (qmail 31158 invoked by alias); 28 Jun 2010 14:17:58 -0000 Received: (qmail 31108 invoked by uid 22791); 28 Jun 2010 14:17:57 -0000 X-SWARE-Spam-Status: No, hits=-4.6 required=5.0 tests=AWL, BAYES_20, RCVD_IN_DNSWL_HI, TW_CP, TW_TM, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from cantor.suse.de (HELO mx1.suse.de) (195.135.220.2) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 28 Jun 2010 14:17:29 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.221.2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.suse.de (Postfix) with ESMTP id ED5A96CB00 for ; Mon, 28 Jun 2010 16:17:26 +0200 (CEST) Date: Mon, 28 Jun 2010 16:17:26 +0200 (CEST) From: Michael Matz To: gcc-patches@gcc.gnu.org Subject: Re: [rfa] Fix PR44592: wrong code In-Reply-To: Message-ID: References: MIME-Version: 1.0 X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org 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. 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