From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31482 invoked by alias); 10 Jul 2014 14:55:05 -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 31427 invoked by uid 89); 10 Jul 2014 14:55:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wi0-f175.google.com Received: from mail-wi0-f175.google.com (HELO mail-wi0-f175.google.com) (209.85.212.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Thu, 10 Jul 2014 14:54:57 +0000 Received: by mail-wi0-f175.google.com with SMTP id ho1so4736919wib.8 for ; Thu, 10 Jul 2014 07:54:53 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.180.36.18 with SMTP id m18mr19626048wij.74.1405004093293; Thu, 10 Jul 2014 07:54:53 -0700 (PDT) Received: by 10.195.11.202 with HTTP; Thu, 10 Jul 2014 07:54:53 -0700 (PDT) In-Reply-To: References: Date: Thu, 10 Jul 2014 14:55:00 -0000 Message-ID: Subject: Re: SRA: don't drop clobbers From: Richard Biener To: Marc Glisse Cc: GCC Patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-07/txt/msg00715.txt.bz2 On Mon, Jun 30, 2014 at 1:38 AM, Marc Glisse wrote: > Hello, > > with this patch on top of > https://gcc.gnu.org/ml/gcc-patches/2014-06/msg02315.html > we finally warn for the testcase of PR 60517. > > The new function is copied from init_subtree_with_zero right above. I guess > it might be possible to merge them into a single function, if desired. I > don't understand the debug stuff, but hopefully by keeping the functions > similar enough it shouldn't be too broken. > > When we see a clobber during scalarization, instead of dropping it, we add a > clobber to the new variable, which the previous patch turns into an SSA_NAME > with a default def. Then either we reach uninit and warn, or the variable > appears in a PHI and CCP optimizes. > > Bootstrap+testsuite (all,obj-c++,ada,go) on x86_64-linux-gnu. > > > 2014-07-01 Marc Glisse > > PR c++/60517 > PR tree-optimization/60770 > gcc/ > * tree-sra.c (clobber_subtree): New function. > (sra_modify_constructor_assign): Call it. > gcc/testsuite/ > * g++.dg/tree-ssa/pr60517.C: New file. > > -- > Marc Glisse > Index: gcc/tree-sra.c > =================================================================== > --- gcc/tree-sra.c (revision 212126) > +++ gcc/tree-sra.c (working copy) > @@ -2727,20 +2727,58 @@ init_subtree_with_zero (struct access *a > if (insert_after) > gsi_insert_after (gsi, ds, GSI_NEW_STMT); > else > gsi_insert_before (gsi, ds, GSI_SAME_STMT); > } > > for (child = access->first_child; child; child = child->next_sibling) > init_subtree_with_zero (child, gsi, insert_after, loc); > } > Missing comment. > +static void > +clobber_subtree (struct access *access, gimple_stmt_iterator *gsi, > + bool insert_after, location_t loc) > + > +{ > + struct access *child; > + > + if (access->grp_to_be_replaced) > + { > + tree rep = get_access_replacement (access); > + tree clobber = build_constructor (access->type, NULL); > + TREE_THIS_VOLATILE (clobber) = 1; > + gimple stmt = gimple_build_assign (rep, clobber); > + > + if (insert_after) > + gsi_insert_after (gsi, stmt, GSI_NEW_STMT); > + else > + gsi_insert_before (gsi, stmt, GSI_SAME_STMT); > + update_stmt (stmt); > + gimple_set_location (stmt, loc); > + } > + else if (access->grp_to_be_debug_replaced) > + { Why would we care to create clobbers for debug stmts?! Are those even valid? Otherwise this looks good I think. Richard. > + tree rep = get_access_replacement (access); > + tree clobber = build_constructor (access->type, NULL); > + TREE_THIS_VOLATILE (clobber) = 1; > + gimple ds = gimple_build_debug_bind (rep, clobber, gsi_stmt (*gsi)); > + > + if (insert_after) > + gsi_insert_after (gsi, ds, GSI_NEW_STMT); > + else > + gsi_insert_before (gsi, ds, GSI_SAME_STMT); > + } > + > + for (child = access->first_child; child; child = child->next_sibling) > + clobber_subtree (child, gsi, insert_after, loc); > +} > + > /* Search for an access representative for the given expression EXPR and > return it or NULL if it cannot be found. */ > > static struct access * > get_access_for_expr (tree expr) > { > HOST_WIDE_INT offset, size, max_size; > tree base; > > /* FIXME: This should not be necessary but Ada produces V_C_Es with a > type of > @@ -3039,43 +3077,41 @@ enum assignment_mod_result { SRA_AM_NONE > SRA_AM_REMOVED }; /* stmt eliminated */ > > /* Modify assignments with a CONSTRUCTOR on their RHS. STMT contains a > pointer > to the assignment and GSI is the statement iterator pointing at it. > Returns > the same values as sra_modify_assign. */ > > static enum assignment_mod_result > sra_modify_constructor_assign (gimple *stmt, gimple_stmt_iterator *gsi) > { > tree lhs = gimple_assign_lhs (*stmt); > - struct access *acc; > - location_t loc; > - > - acc = get_access_for_expr (lhs); > + struct access *acc = get_access_for_expr (lhs); > if (!acc) > return SRA_AM_NONE; > + location_t loc = gimple_location (*stmt); > > if (gimple_clobber_p (*stmt)) > { > - /* Remove clobbers of fully scalarized variables, otherwise > - do nothing. */ > + /* Clobber the replacement variable. */ > + clobber_subtree (acc, gsi, !acc->grp_covered, loc); > + /* Remove clobbers of fully scalarized variables, they are dead. */ > if (acc->grp_covered) > { > unlink_stmt_vdef (*stmt); > gsi_remove (gsi, true); > release_defs (*stmt); > return SRA_AM_REMOVED; > } > else > - return SRA_AM_NONE; > + return SRA_AM_MODIFIED; > } > > - loc = gimple_location (*stmt); > if (vec_safe_length (CONSTRUCTOR_ELTS (gimple_assign_rhs1 (*stmt))) > 0) > { > /* I have never seen this code path trigger but if it can happen the > following should handle it gracefully. */ > if (access_has_children_p (acc)) > generate_subtree_copies (acc->first_child, lhs, acc->offset, 0, 0, > gsi, > true, true, loc); > return SRA_AM_MODIFIED; > } > > Index: gcc/testsuite/g++.dg/tree-ssa/pr60517.C > =================================================================== > --- gcc/testsuite/g++.dg/tree-ssa/pr60517.C (revision 0) > +++ gcc/testsuite/g++.dg/tree-ssa/pr60517.C (working copy) > @@ -0,0 +1,15 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O -Wall" } */ > + > +struct B { > + double x[2]; > +}; > +struct A { > + B b; > + B getB() { return b; } > +}; > + > +double foo(A a) { > + double * x = &(a.getB().x[0]); > + return x[0]; /* { dg-warning "" } */ > +} >