From patchwork Sun Nov 2 10:34:07 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marc Glisse X-Patchwork-Id: 405853 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]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 9AB6C1400A8 for ; Sun, 2 Nov 2014 21:34:21 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:in-reply-to:message-id:references :mime-version:content-type; q=dns; s=default; b=DHdCHZmS8/x7E6kC 42oV++FSWkXFUz72+/+U34KZASEkFqikhdSJ1l/FMiEy76W5lKWMRLFbTR4XATtv R+KidmwZMrr1QHi7MeZEXEo2R2CBSj+U01WAY7zg97DTbivRmRjuVvzXysaOxd2/ WcoDuQVWKlR2Wg3RhQxXK6NsZs4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:in-reply-to:message-id:references :mime-version:content-type; s=default; bh=XbdCfmvzKLmao34HJiuaF/ 9YQFc=; b=rR2k2TIcJtd0oQ41ZKDKeChFhPHbxXvDXCHSX0R4jK72uWI7pyYBfw kwY6TXjkotCohYQ4tJkFBCOIDmHjTIubC/exbRS5izUngIYKSpwJhILm0mEjjoE6 WW0YcUoQ0FTZA4a8+KTnKZMbba+CjbNr3KR1eDyeFntqsRtBoSW18= Received: (qmail 2055 invoked by alias); 2 Nov 2014 10:34:14 -0000 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 Received: (qmail 2045 invoked by uid 89); 2 Nov 2014 10:34:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.3 required=5.0 tests=AWL, BAYES_00, RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mail2-relais-roc.national.inria.fr Received: from mail2-relais-roc.national.inria.fr (HELO mail2-relais-roc.national.inria.fr) (192.134.164.83) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Sun, 02 Nov 2014 10:34:10 +0000 Received: from stedding.saclay.inria.fr (HELO stedding) ([193.55.250.194]) by mail2-relais-roc.national.inria.fr with ESMTP/TLS/AES128-SHA; 02 Nov 2014 11:34:07 +0100 Received: from glisse (helo=localhost) by stedding with local-esmtp (Exim 4.84) (envelope-from ) id 1XksU3-00041a-5m; Sun, 02 Nov 2014 11:34:07 +0100 Date: Sun, 2 Nov 2014 11:34:07 +0100 (CET) From: Marc Glisse To: Richard Biener cc: Jeff Law , GCC Patches Subject: Re: update address taken: don't drop clobbers In-Reply-To: Message-ID: References: <543E9BED.4070905@redhat.com> <543FFFD8.4090406@redhat.com> <6255313F-DDD4-4DA6-B07E-832B251E167F@gmail.com> <54400741.9010203@redhat.com> <544AB4E7.3000900@redhat.com> User-Agent: Alpine 2.02 (DEB 1266 2009-07-14) MIME-Version: 1.0 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. 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. 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; }