From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19419 invoked by alias); 3 Nov 2014 09:06:33 -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 19408 invoked by uid 89); 3 Nov 2014 09:06:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 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-f173.google.com Received: from mail-wi0-f173.google.com (HELO mail-wi0-f173.google.com) (209.85.212.173) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Mon, 03 Nov 2014 09:06:29 +0000 Received: by mail-wi0-f173.google.com with SMTP id n3so5797228wiv.12 for ; Mon, 03 Nov 2014 01:06:26 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.180.12.136 with SMTP id y8mr14761763wib.73.1415005586157; Mon, 03 Nov 2014 01:06:26 -0800 (PST) Received: by 10.194.20.74 with HTTP; Mon, 3 Nov 2014 01:06:26 -0800 (PST) In-Reply-To: References: <543E9BED.4070905@redhat.com> <543FFFD8.4090406@redhat.com> <6255313F-DDD4-4DA6-B07E-832B251E167F@gmail.com> <54400741.9010203@redhat.com> <544AB4E7.3000900@redhat.com> Date: Mon, 03 Nov 2014 09:06:00 -0000 Message-ID: Subject: Re: update address taken: don't drop clobbers From: Richard Biener To: Marc Glisse Cc: Jeff Law , GCC Patches Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-11/txt/msg00043.txt.bz2 On Sun, Nov 2, 2014 at 11:34 AM, Marc Glisse wrote: > On Fri, 31 Oct 2014, Richard Biener wrote: > >> On Sat, Oct 25, 2014 at 6:29 PM, Marc Glisse wrote: >>> >>> On Fri, 24 Oct 2014, Jeff Law wrote: >>> >>>> I'm starting to agree -- a later message indicated you wanted to drop >>>> the >>>> unlink_stmt_vdef call and you wanted to avoid gsi_replace, that seems >>>> fine. >>>> I'll approve once those things are taken care of. >>> >>> >>> >>> The following passed bootstrap+testsuite. I wasn't sure what exactly a >>> clobber is guaranteed not to have (no histograms for instance? At least I >>> assumed it doesn't throw) so I may have kept some unnecessary calls when >>> I >>> inlined gsi_replace. I'd be happy to remove any you feel is useless. >>> >>> 2014-10-26 Marc Glisse >>> >>> PR tree-optimization/60770 >>> gcc/ >>> * tree-into-ssa.c: Include value-prof.h. >>> (maybe_register_def): Replace clobbers with default definitions. >>> * tree-ssa-live.c: Include tree-ssa.h. >>> (set_var_live_on_entry): Do not mark undefined variables as live. >>> (verify_live_on_entry): Do not check undefined variables. >>> * tree-ssa.h (ssa_undefined_value_p): New parameter for the case >>> of partially undefined variables. >>> * tree-ssa.c (ssa_undefined_value_p): Likewise. >>> (execute_update_addresses_taken): Do not drop clobbers. >>> >>> gcc/testsuite/ >>> * gcc.dg/tree-ssa/pr60770-1.c: New file. >>> >>> -- >>> Marc Glisse >>> >>> Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c >>> =================================================================== >>> --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (revision 0) >>> +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (working copy) >>> @@ -0,0 +1,11 @@ >>> +/* { dg-do compile } */ >>> +/* { dg-options "-O -Wall" } */ >>> + >>> +int f(int n){ >>> + int*p; >>> + { >>> + int yyy=n; >>> + p=&yyy; >>> + } >>> + return *p; /* { dg-warning "yyy" } */ >>> +} >>> Index: gcc/tree-into-ssa.c >>> =================================================================== >>> --- gcc/tree-into-ssa.c (revision 216689) >>> +++ gcc/tree-into-ssa.c (working copy) >>> @@ -52,20 +52,21 @@ along with GCC; see the file COPYING3. >>> #include "expr.h" >>> #include "tree-dfa.h" >>> #include "tree-ssa.h" >>> #include "tree-inline.h" >>> #include "tree-pass.h" >>> #include "cfgloop.h" >>> #include "domwalk.h" >>> #include "params.h" >>> #include "diagnostic-core.h" >>> #include "tree-into-ssa.h" >>> +#include "value-prof.h" >>> >>> #define PERCENT(x,y) ((float)(x) * 100.0 / (float)(y)) >>> >>> /* This file builds the SSA form for a function as described in: >>> R. Cytron, J. Ferrante, B. Rosen, M. Wegman, and K. Zadeck. >>> Efficiently >>> Computing Static Single Assignment Form and the Control Dependence >>> Graph. ACM Transactions on Programming Languages and Systems, >>> 13(4):451-490, October 1991. */ >>> >>> /* Structure to map a variable VAR to the set of blocks that contain >>> @@ -1837,26 +1838,42 @@ maybe_register_def (def_operand_p def_p, >>> { >>> tree def = DEF_FROM_PTR (def_p); >>> tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def); >>> >>> /* If DEF is a naked symbol that needs renaming, create a new >>> name for it. */ >>> if (marked_for_renaming (sym)) >>> { >>> if (DECL_P (def)) >>> { >>> - tree tracked_var; >>> + if (gimple_clobber_p (stmt) && is_gimple_reg (sym)) >> >> >> I think you know that sym is a gimple-reg as the code previously >> unconditionally generated an SSA name for it. > > > I checked that in July and it failed: > https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01828.html > >>> + { >>> + /* Replace clobber stmts with a default def. This new use >>> of a >>> + default definition may make it look like SSA_NAMEs have >>> + conflicting lifetimes, so we need special code to let >>> them >>> + coalesce properly. */ >>> + /* Hand-inlined version of the following, for safety >>> + gsi_replace (&gsi, gimple_build_nop (), true); */ >>> + gimple nop = gimple_build_nop (); >>> + gimple_set_bb (nop, gsi_bb (gsi)); >>> + gimple_set_bb (stmt, NULL); >>> + gimple_remove_stmt_histograms (cfun, stmt); >>> + delink_stmt_imm_use (stmt); >>> + gsi_set_stmt (&gsi, nop); >> >> >> Is there any reason for this dance? I'd rather have maybe_register_def >> return a bool whether to remove the stmt... passing it down to the >> single caller of rewrite_update_stmt which can then gsi_remove the >> stmt. > > > For more context, my starting point was the code in rewrite_stmt, which > I was trying to port to rewrite_update_stmt (and thus > maybe_register_def): > > if (gimple_clobber_p (stmt) > && is_gimple_reg (var)) > { > /* If we rewrite a DECL into SSA form then drop its > clobber stmts and replace uses with a new default def. */ > gcc_checking_assert (TREE_CODE (var) == VAR_DECL > && !gimple_vdef (stmt)); > gsi_replace (si, gimple_build_nop (), true); > register_new_def (get_or_create_ssa_default_def (cfun, var), > var); > break; > } > > I would be happy using the same gsi_replace line, but you said that it > is dangerous because it runs update_stmt (though I don't see what bad > thing may happen when replacing a clobber by a nop), so I tried to do > the same thing as gsi_replace without update_stmt... Though now that I > re-read https://gcc.gnu.org/ml/gcc-patches/2014-07/msg01656.html it is not > clear that you are really opposed to the gsi_replace version. > > I tried again with gsi_remove, trying to understand why it was failing > before, and that is because I was calling it in maybe_register_def instead > of delegating to whoever calls gsi_next. > >>> - def = make_ssa_name (def, stmt); >>> + def = get_or_create_ssa_default_def (cfun, sym); >> >> >> I think if 'def' turns out to be a PARM_DECL this does the wrong >> thing (well, not technically wrong... but maybe unexpected). Not >> sure if we ever end up with a PARM = {} clobber though. Maybe >> guard all this with TREE_CODE (def) == VAR_DECL for extra >> safety. > > > Hmm, you are right. The rewrite_stmt version has > > gcc_checking_assert (TREE_CODE (var) == VAR_DECL && ... > > I don't remember exactly why I removed it, maybe because the second part of > the assertion was failing. > > Here is a new version, that passed bootstrap+testsuite on x86_64-linux-gnu. Ok. Thanks, Richard. > 2014-11-03 Marc Glisse > > PR tree-optimization/60770 > gcc/ > * tree-into-ssa.c (rewrite_update_stmt): Return whether the > statement should be removed. > (maybe_register_def): Likewise. Replace clobbers with default > definitions. > (rewrite_dom_walker::before_dom_children): Remove statement if > rewrite_update_stmt says so. > > * tree-ssa-live.c: Include tree-ssa.h. > (set_var_live_on_entry): Do not mark undefined variables as live. > (verify_live_on_entry): Do not check undefined variables. > * tree-ssa.h (ssa_undefined_value_p): New parameter for the case > of partially undefined variables. > * tree-ssa.c (ssa_undefined_value_p): Likewise. > (execute_update_addresses_taken): Do not drop clobbers. > > gcc/testsuite/ > * gcc.dg/tree-ssa/pr60770-1.c: New file. > > -- > Marc Glisse > > Index: gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c > =================================================================== > --- gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (revision 0) > +++ gcc/testsuite/gcc.dg/tree-ssa/pr60770-1.c (working copy) > @@ -0,0 +1,11 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O -Wall" } */ > + > +int f(int n){ > + int*p; > + { > + int yyy=n; > + p=&yyy; > + } > + return *p; /* { dg-warning "yyy" } */ > +} > Index: gcc/tree-into-ssa.c > =================================================================== > --- gcc/tree-into-ssa.c (revision 217007) > +++ gcc/tree-into-ssa.c (working copy) > @@ -1826,41 +1826,51 @@ maybe_replace_use_in_debug_stmt (use_ope > if (rdef && rdef != use) > SET_USE (use_p, rdef); > > return rdef != NULL_TREE; > } > > > /* If the operand pointed to by DEF_P is an SSA name in NEW_SSA_NAMES > or OLD_SSA_NAMES, or if it is a symbol marked for renaming, > register it as the current definition for the names replaced by > - DEF_P. */ > + DEF_P. Returns whether the statement should be removed. */ > > -static inline void > +static inline bool > maybe_register_def (def_operand_p def_p, gimple stmt, > gimple_stmt_iterator gsi) > { > tree def = DEF_FROM_PTR (def_p); > tree sym = DECL_P (def) ? def : SSA_NAME_VAR (def); > + bool to_delete = false; > > /* If DEF is a naked symbol that needs renaming, create a new > name for it. */ > if (marked_for_renaming (sym)) > { > if (DECL_P (def)) > { > - tree tracked_var; > - > - def = make_ssa_name (def, stmt); > + if (gimple_clobber_p (stmt) && is_gimple_reg (sym)) > + { > + gcc_checking_assert (TREE_CODE (sym) == VAR_DECL); > + /* Replace clobber stmts with a default def. This new use of a > + default definition may make it look like SSA_NAMEs have > + conflicting lifetimes, so we need special code to let them > + coalesce properly. */ > + to_delete = true; > + def = get_or_create_ssa_default_def (cfun, sym); > + } > + else > + def = make_ssa_name (def, stmt); > SET_DEF (def_p, def); > > - tracked_var = target_for_debug_bind (sym); > + tree tracked_var = target_for_debug_bind (sym); > if (tracked_var) > { > gimple note = gimple_build_debug_bind (tracked_var, def, > stmt); > /* If stmt ends the bb, insert the debug stmt on the single > non-EH edge from the stmt. */ > if (gsi_one_before_end_p (gsi) && stmt_ends_bb_p (stmt)) > { > basic_block bb = gsi_bb (gsi); > edge_iterator ei; > edge e, ef = NULL; > @@ -1904,40 +1914,42 @@ maybe_register_def (def_operand_p def_p, > /* If DEF is a new name, register it as a new definition > for all the names replaced by DEF. */ > if (is_new_name (def)) > register_new_update_set (def, names_replaced_by (def)); > > /* If DEF is an old name, register DEF as a new > definition for itself. */ > if (is_old_name (def)) > register_new_update_single (def, def); > } > + > + return to_delete; > } > > > /* Update every variable used in the statement pointed-to by SI. The > statement is assumed to be in SSA form already. Names in > OLD_SSA_NAMES used by SI will be updated to their current reaching > definition. Names in OLD_SSA_NAMES or NEW_SSA_NAMES defined by SI > will be registered as a new definition for their corresponding name > - in OLD_SSA_NAMES. */ > + in OLD_SSA_NAMES. Returns whether STMT should be removed. */ > > -static void > +static bool > rewrite_update_stmt (gimple stmt, gimple_stmt_iterator gsi) > { > use_operand_p use_p; > def_operand_p def_p; > ssa_op_iter iter; > > /* Only update marked statements. */ > if (!rewrite_uses_p (stmt) && !register_defs_p (stmt)) > - return; > + return false; > > if (dump_file && (dump_flags & TDF_DETAILS)) > { > fprintf (dump_file, "Updating SSA information for statement "); > print_gimple_stmt (dump_file, stmt, 0, TDF_SLIM); > } > > /* Rewrite USES included in OLD_SSA_NAMES and USES whose underlying > symbol is marked for renaming. */ > if (rewrite_uses_p (stmt)) > @@ -1974,23 +1986,26 @@ rewrite_update_stmt (gimple stmt, gimple > else > { > FOR_EACH_SSA_USE_OPERAND (use_p, stmt, iter, SSA_OP_ALL_USES) > maybe_replace_use (use_p); > } > } > > /* Register definitions of names in NEW_SSA_NAMES and OLD_SSA_NAMES. > Also register definitions for names whose underlying symbol is > marked for renaming. */ > + bool to_delete = false; > if (register_defs_p (stmt)) > FOR_EACH_SSA_DEF_OPERAND (def_p, stmt, iter, SSA_OP_ALL_DEFS) > - maybe_register_def (def_p, stmt, gsi); > + to_delete |= maybe_register_def (def_p, stmt, gsi); > + > + return to_delete; > } > > > /* Visit all the successor blocks of BB looking for PHI nodes. For > every PHI node found, check if any of its arguments is in > OLD_SSA_NAMES. If so, and if the argument has a current reaching > definition, replace it. */ > > static void > rewrite_update_phi_arguments (basic_block bb) > @@ -2142,22 +2157,25 @@ rewrite_update_dom_walker::before_dom_ch > } > > if (is_abnormal_phi) > SSA_NAME_OCCURS_IN_ABNORMAL_PHI (lhs) = 1; > } > > /* Step 2. Rewrite every variable used in each statement in the block. > */ > if (bitmap_bit_p (interesting_blocks, bb->index)) > { > gcc_checking_assert (bitmap_bit_p (blocks_to_update, bb->index)); > - for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) > - rewrite_update_stmt (gsi_stmt (gsi), gsi); > + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); ) > + if (rewrite_update_stmt (gsi_stmt (gsi), gsi)) > + gsi_remove (&gsi, true); > + else > + gsi_next (&gsi); > } > > /* Step 3. Update PHI nodes. */ > rewrite_update_phi_arguments (bb); > } > > /* Called after visiting block BB. Unwind BLOCK_DEFS_STACK to restore > the current reaching definition of every name re-written in BB to > the original reaching definition before visiting BB. This > unwinding must be done in the opposite order to what is done in > Index: gcc/tree-ssa-live.c > =================================================================== > --- gcc/tree-ssa-live.c (revision 217007) > +++ gcc/tree-ssa-live.c (working copy) > @@ -50,20 +50,21 @@ along with GCC; see the file COPYING3. > #include "stringpool.h" > #include "tree-ssanames.h" > #include "expr.h" > #include "tree-dfa.h" > #include "timevar.h" > #include "dumpfile.h" > #include "tree-ssa-live.h" > #include "diagnostic-core.h" > #include "debug.h" > #include "flags.h" > +#include "tree-ssa.h" > > #ifdef ENABLE_CHECKING > static void verify_live_on_entry (tree_live_info_p); > #endif > > > /* VARMAP maintains a mapping from SSA version number to real variables. > > All SSA_NAMES are divided into partitions. Initially each ssa_name is > the > only member of it's own partition. Coalescing will attempt to group any > @@ -1096,20 +1097,24 @@ set_var_live_on_entry (tree ssa_name, tr > if (stmt) > { > def_bb = gimple_bb (stmt); > /* Mark defs in liveout bitmap temporarily. */ > if (def_bb) > bitmap_set_bit (&live->liveout[def_bb->index], p); > } > else > def_bb = ENTRY_BLOCK_PTR_FOR_FN (cfun); > > + /* An undefined local variable does not need to be very alive. */ > + if (ssa_undefined_value_p (ssa_name, false)) > + return; > + > /* Visit each use of SSA_NAME and if it isn't in the same block as the > def, > add it to the list of live on entry blocks. */ > FOR_EACH_IMM_USE_FAST (use, imm_iter, ssa_name) > { > gimple use_stmt = USE_STMT (use); > basic_block add_block = NULL; > > if (gimple_code (use_stmt) == GIMPLE_PHI) > { > /* Uses in PHI's are considered to be live at exit of the SRC > block > @@ -1432,20 +1437,25 @@ verify_live_on_entry (tree_live_info_p l > fprintf (stderr, "\n"); > } > else > fprintf (stderr, " and there is no default def.\n"); > } > } > } > else > if (d == var) > { > + /* An undefined local variable does not need to be very > + alive. */ > + if (ssa_undefined_value_p (var, false)) > + continue; > + > /* The only way this var shouldn't be marked live on entry > is > if it occurs in a PHI argument of the block. */ > size_t z; > bool ok = false; > gimple_stmt_iterator gsi; > for (gsi = gsi_start_phis (e->dest); > !gsi_end_p (gsi) && !ok; > gsi_next (&gsi)) > { > gimple phi = gsi_stmt (gsi); > Index: gcc/tree-ssa.c > =================================================================== > --- gcc/tree-ssa.c (revision 217007) > +++ gcc/tree-ssa.c (working copy) > @@ -1181,24 +1181,25 @@ tree_ssa_useless_type_conversion (tree e > > tree > tree_ssa_strip_useless_type_conversions (tree exp) > { > while (tree_ssa_useless_type_conversion (exp)) > exp = TREE_OPERAND (exp, 0); > return exp; > } > > > -/* Return true if T, an SSA_NAME, has an undefined value. */ > +/* Return true if T, an SSA_NAME, has an undefined value. PARTIAL is what > + should be returned if the value is only partially undefined. */ > > bool > -ssa_undefined_value_p (tree t) > +ssa_undefined_value_p (tree t, bool partial) > { > gimple def_stmt; > tree var = SSA_NAME_VAR (t); > > if (!var) > ; > /* Parameters get their initial value from the function entry. */ > else if (TREE_CODE (var) == PARM_DECL) > return false; > /* When returning by reference the return address is actually a hidden > @@ -1208,21 +1209,21 @@ ssa_undefined_value_p (tree t) > /* Hard register variables get their initial value from the ether. */ > else if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var)) > return false; > > /* The value is undefined iff its definition statement is empty. */ > def_stmt = SSA_NAME_DEF_STMT (t); > if (gimple_nop_p (def_stmt)) > return true; > > /* Check if the complex was not only partially defined. */ > - if (is_gimple_assign (def_stmt) > + if (partial && is_gimple_assign (def_stmt) > && gimple_assign_rhs_code (def_stmt) == COMPLEX_EXPR) > { > tree rhs1, rhs2; > > rhs1 = gimple_assign_rhs1 (def_stmt); > rhs2 = gimple_assign_rhs2 (def_stmt); > return (TREE_CODE (rhs1) == SSA_NAME && ssa_undefined_value_p (rhs1)) > || (TREE_CODE (rhs2) == SSA_NAME && ssa_undefined_value_p > (rhs2)); > } > return false; > @@ -1554,32 +1555,20 @@ execute_update_addresses_taken (void) > rhs = gimple_assign_rhs1 (stmt); > if (gimple_assign_lhs (stmt) != lhs > && !useless_type_conversion_p (TREE_TYPE (lhs), > TREE_TYPE (rhs))) > rhs = fold_build1 (VIEW_CONVERT_EXPR, > TREE_TYPE (lhs), rhs); > > if (gimple_assign_lhs (stmt) != lhs) > gimple_assign_set_lhs (stmt, lhs); > > - /* For var ={v} {CLOBBER}; where var lost > - TREE_ADDRESSABLE just remove the stmt. */ > - if (DECL_P (lhs) > - && TREE_CLOBBER_P (rhs) > - && bitmap_bit_p (suitable_for_renaming, DECL_UID (lhs))) > - { > - unlink_stmt_vdef (stmt); > - gsi_remove (&gsi, true); > - release_defs (stmt); > - continue; > - } > - > if (gimple_assign_rhs1 (stmt) != rhs) > { > gimple_stmt_iterator gsi = gsi_for_stmt (stmt); > gimple_assign_set_rhs_from_tree (&gsi, rhs); > } > } > > else if (gimple_code (stmt) == GIMPLE_CALL) > { > unsigned i; > Index: gcc/tree-ssa.h > =================================================================== > --- gcc/tree-ssa.h (revision 217007) > +++ gcc/tree-ssa.h (working copy) > @@ -44,21 +44,21 @@ extern tree target_for_debug_bind (tree) > extern void insert_debug_temp_for_var_def (gimple_stmt_iterator *, tree); > extern void insert_debug_temps_for_defs (gimple_stmt_iterator *); > extern void reset_debug_uses (gimple); > extern void release_defs_bitset (bitmap toremove); > extern void verify_ssa (bool, bool); > extern void init_tree_ssa (struct function *); > extern void delete_tree_ssa (void); > extern bool tree_ssa_useless_type_conversion (tree); > extern tree tree_ssa_strip_useless_type_conversions (tree); > > -extern bool ssa_undefined_value_p (tree); > +extern bool ssa_undefined_value_p (tree, bool = true); > extern void execute_update_addresses_taken (void); > > /* Given an edge_var_map V, return the PHI arg definition. */ > > static inline tree > redirect_edge_var_map_def (edge_var_map *v) > { > return v->def; > } > >