From patchwork Mon Sep 17 14:50:14 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Law X-Patchwork-Id: 970622 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-485775-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="UsmPDir3"; dkim-atps=neutral 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 42DTZG0NPXz9shy for ; Tue, 18 Sep 2018 00:50:28 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:subject:message-id:date:mime-version:content-type; q=dns; s= default; b=nqDXh5/GmPkwK1qZMVWGU7m8z8xuEdW98VcOpO/FCpXY5polae3sp kOEi+pR0hj+NmjH3PSamsoczCWvwaA/CqmOeK5XrAV0LU/lm5bVQ5iR1xtDeqV13 NBobGFcdQoCtEn3wEYBj5Ardt90PrEbW2m+Cn8Fb06J5viu1zrKkjs= 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:from :to:subject:message-id:date:mime-version:content-type; s= default; bh=w1XoSF2nr/l0AP+VBHI0LMVVz9M=; b=UsmPDir3k9KKSeFwuxw3 PkKclGfYMTmFZ1EJkf6nfNCJ63oV27elkZTg2NI41+aoLxtl23ciIWbDmysyMSDM sKrNSi7iSTEdCgNf7v7UCKWGNNRPM1jPHavFTVc6LpfhP+nkh4agkUuJXqM84PNI NN8ZvTo/Uwg4DBs+2O0fzi0= Received: (qmail 96164 invoked by alias); 17 Sep 2018 14:50:20 -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 94836 invoked by uid 89); 17 Sep 2018 14:50:19 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=H*M:3939, H*MI:3939 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 17 Sep 2018 14:50:17 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4141081DE9 for ; Mon, 17 Sep 2018 14:50:16 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-75.rdu2.redhat.com [10.10.112.75]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9269260C5C for ; Mon, 17 Sep 2018 14:50:15 +0000 (UTC) From: Jeff Law Openpgp: preference=signencrypt To: gcc-patches Subject: [RFA] Minor cleanup to VRP/EVRP handling of deferred edge/switch optimization Message-ID: <6aacd581-5c9f-fb7b-3939-67be559d1c00@redhat.com> Date: Mon, 17 Sep 2018 08:50:14 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 MIME-Version: 1.0 X-IsSubscribed: yes This is a relatively minor cleanup that I should have caught last cycle, but somehow missed. We have two structures TO_REMOVE_EDGES and TO_UPDATE_SWITCH_STMTS which are used by the VRP/EVRP code to record edges to remove and switch statements that need updating. They are currently implemented as globals within tree-vrp.c with an appropriate extern within tree-vrp.h. The code to walk those vectors was only implemented in VRP, but we can (and do) add to those vectors within EVRP. So EVRP would detect certain edges as dead or switches that were needed simplification, but they were left as-is because EVRP never walked the vectors to do the necessary cleanup. This change pushes the vectors into the vr_values structure. They're initialized in the ctor and we verify they're properly cleaned up in the dtor. This obviously avoids the global object carrying state, but also catches cases where we record that an optimization was possible but failed to update the IL appropriately. As a side effect, we don't need to bother with initializing and wiping EDGE_IGNORE for jump threading in VRP. We just mark the appropriate edges at the same time we put them in TO_REMOVE_EDGES within vr_values. vrp113.c is an example of where EVRP detected optimizations should be possible, but failed to update the IL. Given the test really wanted to check VRP's behavior, I've disabled EVRP for that test. Bootstrapped and regression tested on x86_64-linux-gnu. OK for the trunk? Jeff * gimple-ssa-evrp.c (evrp_dom_walker::cleanup): Call vr_values::cleanup_edges_and_switches. * tree-vrp.c (to_remove_edges, to_update_switch_stmts): Moved into vr_values class. (identify_jump_threads): Remove EDGE_IGNORE handling. (execute_vrp): Move handling of to_remove_edges and to_update_switch_stmts into vr_values class member functions. * tree-vrp.h (switch_update, to_remove_edges): Remove declarations. (to_update_switch_stmts): Likewise. * vr-values.c: Include cfghooks.h. (vr_values::vr_values): Initialize to_remove_edges and to_update_switch_stmts. (vr_values::~vr_values): Verify to_remove_edges and to_update_switch_stmts are empty. (vr_values::simplify_switch_using_ranges): Set EDGE_IGNORE as needed. (vr_values::cleanup_edges_and_switches): New member function. * vr-values.h (vr_values): Add cleanup_edges_and_switches member function. Add new data members. * gcc.dg/tree-ssa/vrp113.c: Disable EVRP. diff --git a/gcc/gimple-ssa-evrp.c b/gcc/gimple-ssa-evrp.c index b9a054fd2ee..50e8adc1aad 100644 --- a/gcc/gimple-ssa-evrp.c +++ b/gcc/gimple-ssa-evrp.c @@ -287,6 +287,8 @@ evrp_dom_walker::cleanup (void) gimple *stmt = stmts_to_fixup.pop (); fixup_noreturn_call (stmt); } + + evrp_folder.vr_values->cleanup_edges_and_switches (); } /* Main entry point for the early vrp pass which is a simplified non-iterative diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp113.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp113.c index 5069fdfa784..ab8d91e0f10 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/vrp113.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp113.c @@ -1,5 +1,5 @@ /* { dg-do compile } */ -/* { dg-options "-O2 -fdump-tree-vrp1" } */ +/* { dg-options "-O2 -fdump-tree-vrp1 -fdisable-tree-evrp" } */ int f(int a) { switch (a & 1) { diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c index 622ccbc2df7..ab222a385f6 100644 --- a/gcc/tree-vrp.c +++ b/gcc/tree-vrp.c @@ -121,10 +121,6 @@ static bitmap need_assert_for; ASSERT_EXPRs for SSA name N_I should be inserted. */ static assert_locus **asserts_for; -vec to_remove_edges; -vec to_update_switch_stmts; - - /* Return the maximum value for TYPE. */ tree @@ -6404,9 +6400,6 @@ vrp_dom_walker::after_dom_children (basic_block bb) static void identify_jump_threads (class vr_values *vr_values) { - int i; - edge e; - /* Ugh. When substituting values earlier in this pass we can wipe the dominance information. So rebuild the dominator information as we need it within the jump threading code. */ @@ -6419,11 +6412,6 @@ identify_jump_threads (class vr_values *vr_values) recompute it. */ mark_dfs_back_edges (); - /* Do not thread across edges we are about to remove. Just marking - them as EDGE_IGNORE will do. */ - FOR_EACH_VEC_ELT (to_remove_edges, i, e) - e->flags |= EDGE_IGNORE; - /* Allocate our unwinder stack to unwind any temporary equivalences that might be recorded. */ const_and_copies *equiv_stack = new const_and_copies (); @@ -6437,10 +6425,6 @@ identify_jump_threads (class vr_values *vr_values) walker.vr_values = vr_values; walker.walk (cfun->cfg->x_entry_block_ptr); - /* Clear EDGE_IGNORE. */ - FOR_EACH_VEC_ELT (to_remove_edges, i, e) - e->flags &= ~EDGE_IGNORE; - /* We do not actually update the CFG or SSA graphs at this point as ASSERT_EXPRs are still in the IL and cfg cleanup code does not yet handle ASSERT_EXPRs gracefully. */ @@ -6557,9 +6541,6 @@ vrp_prop::vrp_finalize (bool warn_array_bounds_p) static unsigned int execute_vrp (bool warn_array_bounds_p) { - int i; - edge e; - switch_update *su; loop_optimizer_init (LOOPS_NORMAL | LOOPS_HAVE_RECORDED_EXITS); rewrite_into_loop_closed_ssa (NULL, TODO_update_ssa); @@ -6570,8 +6551,6 @@ execute_vrp (bool warn_array_bounds_p) EDGE_DFS_BACK. */ insert_range_assertions (); - to_remove_edges.create (10); - to_update_switch_stmts.create (5); threadedge_initialize_values (); /* For visiting PHI nodes we need EDGE_DFS_BACK computed. */ @@ -6623,35 +6602,7 @@ execute_vrp (bool warn_array_bounds_p) processing by the pass manager. */ thread_through_all_blocks (false); - /* Remove dead edges from SWITCH_EXPR optimization. This leaves the - CFG in a broken state and requires a cfg_cleanup run. */ - FOR_EACH_VEC_ELT (to_remove_edges, i, e) - remove_edge (e); - /* Update SWITCH_EXPR case label vector. */ - FOR_EACH_VEC_ELT (to_update_switch_stmts, i, su) - { - size_t j; - size_t n = TREE_VEC_LENGTH (su->vec); - tree label; - gimple_switch_set_num_labels (su->stmt, n); - for (j = 0; j < n; j++) - gimple_switch_set_label (su->stmt, j, TREE_VEC_ELT (su->vec, j)); - /* As we may have replaced the default label with a regular one - make sure to make it a real default label again. This ensures - optimal expansion. */ - label = gimple_switch_label (su->stmt, 0); - CASE_LOW (label) = NULL_TREE; - CASE_HIGH (label) = NULL_TREE; - } - - if (to_remove_edges.length () > 0) - { - free_dominance_info (CDI_DOMINATORS); - loops_state_set (LOOPS_NEED_FIXUP); - } - - to_remove_edges.release (); - to_update_switch_stmts.release (); + vrp_prop.vr_values.cleanup_edges_and_switches (); threadedge_finalize_values (); scev_finalize (); diff --git a/gcc/tree-vrp.h b/gcc/tree-vrp.h index 2f661613dfc..655cf055f0a 100644 --- a/gcc/tree-vrp.h +++ b/gcc/tree-vrp.h @@ -122,13 +122,4 @@ extern int value_inside_range (tree, tree, tree); extern tree get_single_symbol (tree, bool *, tree *); extern void maybe_set_nonzero_bits (edge, tree); extern value_range_type determine_value_range (tree, wide_int *, wide_int *); - -struct switch_update { - gswitch *stmt; - tree vec; -}; - -extern vec to_remove_edges; -extern vec to_update_switch_stmts; - #endif /* GCC_TREE_VRP_H */ diff --git a/gcc/vr-values.c b/gcc/vr-values.c index 11df1023b6e..e28e259a8b1 100644 --- a/gcc/vr-values.c +++ b/gcc/vr-values.c @@ -47,6 +47,7 @@ along with GCC; see the file COPYING3. If not see #include "alloc-pool.h" #include "attribs.h" #include "vr-values.h" +#include "cfghooks.h" /* Set value range VR to a non-negative range of type TYPE. */ @@ -1918,6 +1919,8 @@ vr_values::vr_values () : vrp_value_range_pool ("Tree VRP value ranges") vr_value = XCNEWVEC (value_range *, num_vr_values); vr_phi_edge_counts = XCNEWVEC (int, num_ssa_names); bitmap_obstack_initialize (&vrp_equiv_obstack); + to_remove_edges.create (10); + to_update_switch_stmts.create (5); } /* Free VRP lattice. */ @@ -1934,6 +1937,12 @@ vr_values::~vr_values () and not available. */ vr_value = NULL; vr_phi_edge_counts = NULL; + + /* If there are entries left in TO_REMOVE_EDGES or TO_UPDATE_SWITCH_STMTS + then an EVRP client did not clean up properly. Catch it now rather + than seeing something more obscure later. */ + gcc_assert (to_remove_edges.length () == 0 + && to_update_switch_stmts.length () == 0); } @@ -3780,6 +3789,7 @@ vr_values::simplify_switch_using_ranges (gswitch *stmt) } to_remove_edges.safe_push (e); e->flags &= ~EDGE_EXECUTABLE; + e->flags |= EDGE_IGNORE; } /* And queue an update for the stmt. */ @@ -3789,6 +3799,48 @@ vr_values::simplify_switch_using_ranges (gswitch *stmt) return false; } +/* Remove any edges in TO_REMOVE_EDGES from the CFG. + Update any switch statements in TO_UPDATE_SWITCH_STMTS. */ + +void +vr_values::cleanup_edges_and_switches (void) +{ + int i; + edge e; + switch_update *su; + + /* Remove dead edges from SWITCH_EXPR optimization. This leaves the + CFG in a broken state and requires a cfg_cleanup run. */ + FOR_EACH_VEC_ELT (to_remove_edges, i, e) + remove_edge (e); + + /* Update SWITCH_EXPR case label vector. */ + FOR_EACH_VEC_ELT (to_update_switch_stmts, i, su) + { + size_t j; + size_t n = TREE_VEC_LENGTH (su->vec); + tree label; + gimple_switch_set_num_labels (su->stmt, n); + for (j = 0; j < n; j++) + gimple_switch_set_label (su->stmt, j, TREE_VEC_ELT (su->vec, j)); + /* As we may have replaced the default label with a regular one + make sure to make it a real default label again. This ensures + optimal expansion. */ + label = gimple_switch_label (su->stmt, 0); + CASE_LOW (label) = NULL_TREE; + CASE_HIGH (label) = NULL_TREE; + } + + if (to_remove_edges.length () > 0) + { + free_dominance_info (CDI_DOMINATORS); + loops_state_set (LOOPS_NEED_FIXUP); + } + + to_remove_edges.release (); + to_update_switch_stmts.release (); +} + /* Simplify an integral conversion from an SSA name in STMT. */ static bool diff --git a/gcc/vr-values.h b/gcc/vr-values.h index 80b16cacefc..ff26d53a079 100644 --- a/gcc/vr-values.h +++ b/gcc/vr-values.h @@ -68,6 +68,10 @@ class vr_values value_range *allocate_value_range (void) { return vrp_value_range_pool.allocate (); } + /* Handle queued cleanups, must be called before the class is + destructed. */ + void cleanup_edges_and_switches (void); + private: void add_equivalence (bitmap *, const_tree); bool vrp_stmt_computes_nonzero (gimple *); @@ -124,6 +128,19 @@ class vr_values number of executable edges we saw the last time we visited the node. */ int *vr_phi_edge_counts; + + /* Vectors of edges that need removing and switch statements that + need updating. It is expected that a pass using the simplification + routines will, at the end of the pass, clean up the edges and + switch statements. The class dtor will try to detect cases + that do not follow that expectation. */ + struct switch_update { + gswitch *stmt; + tree vec; + }; + + vec to_remove_edges; + vec to_update_switch_stmts; }; #define VR_INITIALIZER { VR_UNDEFINED, NULL_TREE, NULL_TREE, NULL }