From patchwork Tue Nov 12 16:42:28 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Law X-Patchwork-Id: 290719 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)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 4773C2C00AD for ; Wed, 13 Nov 2013 03:43:27 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:subject:references :in-reply-to:content-type; q=dns; s=default; b=XXynAw56521oRfEUo 5DAll1SDbLuYaxPiBgN/4wVKwa977nK7XOeXFIWxXwGQ/ox3w614aD4bI4Ohws9L AL5lrCSGV54+lyzHRvfn/wsrobtlMmSQWb4HvysITbzRCHuGdCaCl+pKUdZG67vc qtXD9lpQsiKLwiDsjyYIU1PnWE= 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 :message-id:date:from:mime-version:to:subject:references :in-reply-to:content-type; s=default; bh=HcEvXulzPPQfjfL80GI3IEu DXDs=; b=bqFswi70TAMxXuKo9SAGe0qt5ERACHDvBlqlZqxaEOsDFrtJKJ5ENNB 8NEYxelIkgivvsW6XKd6pB9nFXXUa/VDsqWHDlz5gZl2n6Y2xOquIehyismtuas+ hCGEdHNhtP19EOscczIeB9GHEn751Vj/3zgf7ZxaU4kFz+R+bSho= Received: (qmail 28843 invoked by alias); 12 Nov 2013 16:43:10 -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 28814 invoked by uid 89); 12 Nov 2013 16:43:10 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.7 required=5.0 tests=AWL, BAYES_50, RDNS_NONE autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Nov 2013 16:42:37 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rACGgTim022088 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 12 Nov 2013 11:42:29 -0500 Received: from stumpy.slc.redhat.com (ovpn-113-132.phx2.redhat.com [10.3.113.132]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id rACGgSJs019999 for ; Tue, 12 Nov 2013 11:42:29 -0500 Message-ID: <52825A74.8070806@redhat.com> Date: Tue, 12 Nov 2013 09:42:28 -0700 From: Jeff Law User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.0 MIME-Version: 1.0 To: gcc-patches@gcc.gnu.org Subject: [PATCH] Isolate erroneous paths optimization -- preserve *0. References: <5271F493.4020308@redhat.com> <1586972.iyObK3nBib@polaris> <52804ACB.9010206@redhat.com> In-Reply-To: <52804ACB.9010206@redhat.com> X-IsSubscribed: yes On 11/10/13 20:11, Jeff Law wrote: > OK. It sounds like there's a pretty general consensus that we ought ot > go ahead and leave in a load/store of a NULL pointer. I'll go ahead and > run with that. I'll probably just emit SSA_NAME = *0, unless someone > thinks we ought ot distinguish between loads and stores, emitting > SSA_NAME = *0 and *0 = 0 for the two cases respectively. [ ... ] Here's a patch which arranges to leave a null pointer dereferece in the IL. For a memory load through a null pointer, we just leave the statement as-is since the LHS is going to be a throw-away SSA_NAME. For a store through a null pointer, we try to simplify the RHS when it's trivially easy to do so (when the RHS is an integral type). This allows DCE to sometimes do a better job if the RHS is something that was computed on the isolated path. This could be easily extended to floating types, vectors and structures if someone is interested. We issue the builtin_trap after the null dereference. For other cases (invalid NULL argument, invalid NULL return), we continue to issue the builtin trap in the same manner we did before. I've adjusted the existing tests and added a new one which verifies the RHS of a store through a null pointer is simplified. Bootstrapped and regression tested on x86_64-unknown-linux-gnu. Installed on the trunk. * gimple-ssa-isolate-paths.c (check_loadstore): New function. (insert_trap_and_remove_trailing_statements): New argument OP which is the NULL pointer. Emit the trap after the load/store through the NULL pointer. Simplify the RHS of a store through a NULL pointer when trivial to do so. (isolate_path): Corresponding changes. (gimple_ssa_isolate_erroneous_path): Likewise. * gcc.dg/tree-ssa/isolate-1.c: Update expected output. * gcc.dg/tree-ssa/isolate-5.c: New test. diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c index 983ed4d..1203cfc 100644 --- a/gcc/gimple-ssa-isolate-paths.c +++ b/gcc/gimple-ssa-isolate-paths.c @@ -39,17 +39,65 @@ along with GCC; see the file COPYING3. If not see static bool cfg_altered; -/* Insert a trap before SI and remove SI and all statements after SI. */ +/* Callback for walk_stmt_load_store_ops. + + Return TRUE if OP will dereference the tree stored in DATA, FALSE + otherwise. + + This routine only makes a superficial check for a dereference. Thus, + it must only be used if it is safe to return a false negative. */ +static bool +check_loadstore (gimple stmt ATTRIBUTE_UNUSED, tree op, void *data) +{ + if ((TREE_CODE (op) == MEM_REF || TREE_CODE (op) == TARGET_MEM_REF) + && operand_equal_p (TREE_OPERAND (op, 0), (tree)data, 0)) + return true; + return false; +} + +/* Insert a trap after SI and remove SI and all statements after the trap. */ static void -insert_trap_and_remove_trailing_statements (gimple_stmt_iterator *si_p) +insert_trap_and_remove_trailing_statements (gimple_stmt_iterator *si_p, tree op) { - gimple_seq seq = NULL; - gimple stmt = gimple_build_call (builtin_decl_explicit (BUILT_IN_TRAP), 0); - gimple_seq_add_stmt (&seq, stmt); - gsi_insert_before (si_p, seq, GSI_SAME_STMT); + /* We want the NULL pointer dereference to actually occur so that + code that wishes to catch the signal can do so. - /* Now delete all remaining statements in this block. */ + If the dereference is a load, then there's nothing to do as the + LHS will be a throw-away SSA_NAME and the LHS is the NULL dereference. + + If the dereference is a store and we can easily transform the RHS, + then simplify the RHS to enable more DCE. */ + gimple stmt = gsi_stmt (*si_p); + if (walk_stmt_load_store_ops (stmt, (void *)op, NULL, check_loadstore) + && INTEGRAL_TYPE_P (TREE_TYPE (gimple_assign_lhs (stmt)))) + { + /* We just need to turn the RHS into zero converted to the proper + type. */ + tree type = TREE_TYPE (gimple_assign_lhs (stmt)); + gimple_assign_set_rhs_code (stmt, INTEGER_CST); + gimple_assign_set_rhs1 (stmt, fold_convert (type, integer_zero_node)); + update_stmt (stmt); + } + + gimple new_stmt + = gimple_build_call (builtin_decl_explicit (BUILT_IN_TRAP), 0); + gimple_seq seq = NULL; + gimple_seq_add_stmt (&seq, new_stmt); + + /* If we had a NULL pointer dereference, then we want to insert the + __builtin_trap after the statement, for the other cases we want + to insert before the statement. */ + if (walk_stmt_load_store_ops (stmt, (void *)op, + check_loadstore, + check_loadstore)) + gsi_insert_after (si_p, seq, GSI_NEW_STMT); + else + gsi_insert_before (si_p, seq, GSI_NEW_STMT); + + /* The iterator points to the __builtin_trap. Advance the iterator + and delete everything else in the block. */ + gsi_next (si_p); for (; !gsi_end_p (*si_p);) { stmt = gsi_stmt (*si_p); @@ -73,7 +121,8 @@ insert_trap_and_remove_trailing_statements (gimple_stmt_iterator *si_p) Return BB'. */ basic_block -isolate_path (basic_block bb, basic_block duplicate, edge e, gimple stmt) +isolate_path (basic_block bb, basic_block duplicate, + edge e, gimple stmt, tree op) { gimple_stmt_iterator si, si2; edge_iterator ei; @@ -133,7 +182,7 @@ isolate_path (basic_block bb, basic_block duplicate, edge e, gimple stmt) SI2 points to the duplicate of STMT in DUPLICATE. Insert a trap before SI2 and remove SI2 and all trailing statements. */ if (!gsi_end_p (si2)) - insert_trap_and_remove_trailing_statements (&si2); + insert_trap_and_remove_trailing_statements (&si2, op); return duplicate; } @@ -224,7 +273,7 @@ gimple_ssa_isolate_erroneous_paths (void) if (infer_nonnull_range (use_stmt, lhs)) { duplicate = isolate_path (bb, duplicate, - e, use_stmt); + e, use_stmt, lhs); /* When we remove an incoming edge, we need to reprocess the Ith element. */ @@ -247,7 +296,8 @@ gimple_ssa_isolate_erroneous_paths (void) where a non-NULL value is required. */ if (infer_nonnull_range (stmt, null_pointer_node)) { - insert_trap_and_remove_trailing_statements (&si); + insert_trap_and_remove_trailing_statements (&si, + null_pointer_node); /* And finally, remove all outgoing edges from BB. */ edge e; diff --git a/gcc/testsuite/gcc.dg/tree-ssa/isolate-1.c b/gcc/testsuite/gcc.dg/tree-ssa/isolate-1.c index 6b779b4..33745ba 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/isolate-1.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/isolate-1.c @@ -43,12 +43,14 @@ d_type (struct d_info *di) return ret; } -/* We're testing two aspects of isolation here. First that isolation +/* We're testing three aspects of isolation here. First that isolation occurs, second that if we have two null dereferences in a block that that we delete everything from the first dereferece to the end of the - block, regardless of which comes first in the immediate use iterator. */ + block, regardless of which comes first in the immediate use iterator + and finally that we set the RHS of the store to zero. */ /* { dg-final { scan-tree-dump-times "__builtin_trap" 1 "isolate-paths"} } */ -/* { dg-final { scan-tree-dump-times "->type" 1 "isolate-paths"} } */ +/* { dg-final { scan-tree-dump-times "->type = 42" 1 "isolate-paths"} } */ +/* { dg-final { scan-tree-dump-times "->type = 0" 1 "isolate-paths"} } */ /* { dg-final { scan-tree-dump-times "->zzz" 1 "isolate-paths"} } */ /* { dg-final { cleanup-tree-dump "isolate-paths" } } */ diff --git a/gcc/testsuite/gcc.dg/tree-ssa/isolate-5.c b/gcc/testsuite/gcc.dg/tree-ssa/isolate-5.c new file mode 100644 index 0000000..a70871e --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/isolate-5.c @@ -0,0 +1,60 @@ + +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-tree-isolate-paths" } */ + + +struct demangle_component +{ + + int type; + int zzz; + +}; + + +struct d_info +{ + struct demangle_component *comps; + int next_comp; + int num_comps; +}; + + +static struct demangle_component * +d_make_empty (struct d_info *di) +{ + struct demangle_component *p; + + if (di->next_comp >= di->num_comps) + return ((void *)0); + p = &di->comps[di->next_comp]; + return p; +} + + + +struct demangle_component * +d_type (struct d_info *di) +{ + struct demangle_component *ret; + ret = d_make_empty (di); + foo (ret->type); + bar (ret->zzz); + return ret; +} + +/* We're testing two aspects of isolation here. First that isolation + occurs, second that if we have two null dereferences in a block that + that we delete everything from the first dereferece to the end of the + block, regardless of which comes first in the immediate use iterator. + + We leave the 0->type in the IL, so expect to see ->type twice. */ +/* { dg-final { scan-tree-dump-times "__builtin_trap" 1 "isolate-paths"} } */ +/* { dg-final { scan-tree-dump-times "->type" 2 "isolate-paths"} } */ +/* { dg-final { scan-tree-dump-times "->zzz" 1 "isolate-paths"} } */ +/* { dg-final { cleanup-tree-dump "isolate-paths" } } */ + + + + +