From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18035 invoked by alias); 16 Apr 2013 12:02:07 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 18024 invoked by uid 89); 16 Apr 2013 12:02:07 -0000 X-Spam-SWARE-Status: No, score=-6.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,TW_TM autolearn=ham version=3.3.1 Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 16 Apr 2013 12:02:06 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 69C0FA5555 for ; Tue, 16 Apr 2013 14:02:04 +0200 (CEST) Date: Tue, 16 Apr 2013 16:39:00 -0000 From: Richard Biener To: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Fix PR56756 In-Reply-To: Message-ID: References: User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-SW-Source: 2013-04/txt/msg00992.txt.bz2 On Thu, 28 Mar 2013, Richard Biener wrote: > > The domwalker fix to order dom children after inverted postorder > exposed the latent issue that LIM relies on domwalk to walk > all blocks defining SSA names before all blocks using them ... > which is what the following patch tries to fix using the > dependency information it already tracks (but incompletely so, > thus the fixes). > > Bootstrapped and tested on x86_64-unknown-linux-gnu. > > I'm not entirely happy with this - it should use a worklist > of stmts instead of recursing and handling basic-blocks. > But well ... > > Leaving for comments. (inverted_post_order_compute visits > loop nodes in "weird" order because it visits loop exit > nodes last) Ok, I wasn't really happy with the above. The following simpler patch avoids creating intermediate IL that has SSA uses where their definition does not dominate the use. Simply by placing the store-motion load before a random previous load/store where obviously all SSA names used to compute the load/store address have dominating definitions. That isn't guaranteed when inserting into the loop latch block as the testcase shows. Bootstrap and regtest pending on x86_64-unknown-linux-gnu. Richard. 2013-04-16 Richard Biener PR tree-optimization/56756 * tree-ssa-loop-im.c (struct first_mem_ref_loc_1): New functor. (first_mem_ref_loc): New. (execute_sm): Place the load temporarily before a previous access instead of in the latch edge to ensure its SSA dependencies are defined at points dominating the load. * gcc.dg/torture/pr56756.c: New testcase. Index: gcc/tree-ssa-loop-im.c =================================================================== *** gcc/tree-ssa-loop-im.c (revision 197997) --- gcc/tree-ssa-loop-im.c (working copy) *************** rewrite_mem_refs (struct loop *loop, mem *** 1718,1723 **** --- 1718,1749 ---- for_all_locs_in_loop (loop, ref, rewrite_mem_ref_loc (tmp_var)); } + /* Stores the first reference location in LOCP. */ + + struct first_mem_ref_loc_1 + { + first_mem_ref_loc_1 (mem_ref_loc_p *locp_) : locp (locp_) {} + bool operator()(mem_ref_loc_p loc); + mem_ref_loc_p *locp; + }; + + bool + first_mem_ref_loc_1::operator()(mem_ref_loc_p loc) + { + *locp = loc; + return true; + } + + /* Returns the first reference location to REF in LOOP. */ + + static mem_ref_loc_p + first_mem_ref_loc (struct loop *loop, mem_ref_p ref) + { + mem_ref_loc_p locp = NULL; + for_all_locs_in_loop (loop, ref, first_mem_ref_loc_1 (&locp)); + return locp; + } + /* The name and the length of the currently generated variable for lsm. */ #define MAX_LSM_NAME_LENGTH 40 *************** execute_sm (struct loop *loop, vec *** 2022,2030 **** unsigned i; gimple load; struct fmt_data fmt_data; ! edge ex, latch_edge; struct lim_aux_data *lim_data; bool multi_threaded_model_p = false; if (dump_file && (dump_flags & TDF_DETAILS)) { --- 2048,2057 ---- unsigned i; gimple load; struct fmt_data fmt_data; ! edge ex; struct lim_aux_data *lim_data; bool multi_threaded_model_p = false; + gimple_stmt_iterator gsi; if (dump_file && (dump_flags & TDF_DETAILS)) { *************** execute_sm (struct loop *loop, vec *** 2049,2057 **** rewrite_mem_refs (loop, ref, tmp_var); ! /* Emit the load code into the latch, so that we are sure it will ! be processed after all dependencies. */ ! latch_edge = loop_latch_edge (loop); /* FIXME/TODO: For the multi-threaded variant, we could avoid this load altogether, since the store is predicated by a flag. We --- 2076,2085 ---- rewrite_mem_refs (loop, ref, tmp_var); ! /* Emit the load code on a random exit edge or into the latch if ! the loop does not exit, so that we are sure it will be processed ! by move_computations after all dependencies. */ ! gsi = gsi_for_stmt (first_mem_ref_loc (loop, ref)->stmt); /* FIXME/TODO: For the multi-threaded variant, we could avoid this load altogether, since the store is predicated by a flag. We *************** execute_sm (struct loop *loop, vec *** 2060,2066 **** lim_data = init_lim_data (load); lim_data->max_loop = loop; lim_data->tgt_loop = loop; ! gsi_insert_on_edge (latch_edge, load); if (multi_threaded_model_p) { --- 2088,2094 ---- lim_data = init_lim_data (load); lim_data->max_loop = loop; lim_data->tgt_loop = loop; ! gsi_insert_before (&gsi, load, GSI_SAME_STMT); if (multi_threaded_model_p) { *************** execute_sm (struct loop *loop, vec *** 2068,2074 **** lim_data = init_lim_data (load); lim_data->max_loop = loop; lim_data->tgt_loop = loop; ! gsi_insert_on_edge (latch_edge, load); } /* Sink the store to every exit from the loop. */ --- 2096,2102 ---- lim_data = init_lim_data (load); lim_data->max_loop = loop; lim_data->tgt_loop = loop; ! gsi_insert_before (&gsi, load, GSI_SAME_STMT); } /* Sink the store to every exit from the loop. */ Index: gcc/testsuite/gcc.dg/torture/pr56756.c =================================================================== *** gcc/testsuite/gcc.dg/torture/pr56756.c (revision 0) --- gcc/testsuite/gcc.dg/torture/pr56756.c (working copy) *************** *** 0 **** --- 1,28 ---- + /* { dg-do compile } */ + + int a, *b; + + void f(void) + { + if(a) + { + int k; + + for(a = 0; a < 1; a++) + { + int **q; + f(); + + for(; **q; ++**q) + lbl: + if(a) + { + a = 0; + goto lbl; + } + + b = &k; + } + } + goto lbl; + }