From patchwork Fri Jun 6 07:00:25 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Ilya Enkovich X-Patchwork-Id: 356701 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 2FF5714007B for ; Fri, 6 Jun 2014 17:00:52 +1000 (EST) 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:message-id:references:mime-version :content-type:in-reply-to; q=dns; s=default; b=mrnhLjuthFEBtua21 wP4j0Hx3qjNdzMpcZaO89vuKmK1/ZJpjp1iuHExJXVIYf3tbPF93pp2eRvySx+Mn WCK2AwZXIMOTel6q7cuYb9Qa7ceb2zjrU5O9Kkgb5Q2g+pqDejWw4IST2Bt4uaDH IMea6yek+0WJHPCMdXi3yIYjK0= 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:message-id:references:mime-version :content-type:in-reply-to; s=default; bh=2cjL8BHuAJZJyGJI//sWlxY OuHE=; b=gkiOvVE3spCQf9CPudxuE3M+WdagCtBILfS4dhgroc7NvHY8MddVySU drcz2wKC90fuitkJmTBwW5sEWnjYW0GX+Tfu3PMmpceofxdrFDrCBvYUe86Zwn6t sLSwIhelo5RgmzaKLTv4Rt5rKFhn4/tEwfve3FEFoVdhhSFDGiZ8= Received: (qmail 29367 invoked by alias); 6 Jun 2014 07:00:45 -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 29351 invoked by uid 89); 6 Jun 2014 07:00:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qa0-f45.google.com Received: from mail-qa0-f45.google.com (HELO mail-qa0-f45.google.com) (209.85.216.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 06 Jun 2014 07:00:43 +0000 Received: by mail-qa0-f45.google.com with SMTP id hw13so3075384qab.32 for ; Fri, 06 Jun 2014 00:00:40 -0700 (PDT) X-Received: by 10.140.98.234 with SMTP id o97mr4853917qge.35.1402038040875; Fri, 06 Jun 2014 00:00:40 -0700 (PDT) Received: from msticlxl57.ims.intel.com ([192.55.54.40]) by mx.google.com with ESMTPSA id ja8sm31883466pbd.3.2014.06.06.00.00.29 for (version=TLSv1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Fri, 06 Jun 2014 00:00:40 -0700 (PDT) Date: Fri, 6 Jun 2014 11:00:25 +0400 From: Ilya Enkovich To: Richard Biener Cc: GCC Patches Subject: Re: [PATCH, Pointer Bounds Checker 25/x] DCE Message-ID: <20140606070025.GB65215@msticlxl57.ims.intel.com> References: <20140603072340.GC20877@msticlxl57.ims.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes On 03 Jun 17:27, Ilya Enkovich wrote: > 2014-06-03 15:56 GMT+04:00 Richard Biener : > > On Tue, Jun 3, 2014 at 1:36 PM, Ilya Enkovich wrote: > >> 2014-06-03 13:45 GMT+04:00 Richard Biener : > >>> On Tue, Jun 3, 2014 at 9:23 AM, Ilya Enkovich wrote: > >>>> Hi, > >>>> > >>>> This patch adjusts alloc-free removal algorithm in DCE to take into account BUILT_IN_CHKP_BNDRET call returning bounds of allocated memory. > >>>> > >>>> Bootstrapped and tested on linux-x86_64. > >>>> > >>>> Thanks, > >>>> Ilya > >>>> -- > >>>> gcc/ > >>>> > >>>> 2014-06-03 Ilya Enkovich > >>>> > >>>> * tree-ssa-dce.c: Include target.h. > >>>> (propagate_necessity): For free call fed by alloc check > >>>> bounds are also provided by the same alloc. > >>>> (eliminate_unnecessary_stmts): Handle BUILT_IN_CHKP_BNDRET > >>>> used by free calls. > >>>> > >>>> diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c > >>>> index 13a71ce..59a0b71 100644 > >>>> --- a/gcc/tree-ssa-dce.c > >>>> +++ b/gcc/tree-ssa-dce.c > >>>> @@ -73,6 +73,7 @@ along with GCC; see the file COPYING3. If not see > >>>> #include "flags.h" > >>>> #include "cfgloop.h" > >>>> #include "tree-scalar-evolution.h" > >>>> +#include "target.h" > >>>> > >>>> static struct stmt_stats > >>>> { > >>>> @@ -778,7 +779,23 @@ propagate_necessity (bool aggressive) > >>>> && DECL_BUILT_IN_CLASS (def_callee) == BUILT_IN_NORMAL > >>>> && (DECL_FUNCTION_CODE (def_callee) == BUILT_IN_MALLOC > >>>> || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC)) > >>>> - continue; > >>>> + { > >>>> + tree retfndecl > >>>> + = targetm.builtin_chkp_function (BUILT_IN_CHKP_BNDRET); > >>>> + gimple bounds_def_stmt; > >>>> + tree bounds; > >>>> + > >>>> + /* For instrumented calls we should also check used > >>>> + bounds are returned by the same allocation call. */ > >>>> + if (!gimple_call_with_bounds_p (stmt) > >>>> + || ((bounds = gimple_call_arg (stmt, 1)) > >>> > >>> Please provide an abstraction for this - I seem to recall seeing multiple > >>> (variants) of this. Esp. repeated calls to the target hook above look > >>> expensive to me (generally it will not be needed). > >>> > >>> I'd like to see gimple_call_bndarg (stmt) to get rid of the "magic" 1 and I'd > >>> like to see sth similar to gimple_call_builtin_p, for example > >>> gimple_call_chkp_p (stmt, BUILT_IN_CHKP_BNDRET) doing the > >>> target hook invocation and the fndecl check. > >> > >> I do not see how to get rid of constant in gimple_call_arg call here. > >> What semantics does proposed gimple_call_bndarg have? It has to return > >> the first bounds arg but it's not clear from its name and function > >> seems to be very specialized. > > > > Ah, ok. I see now, so the code is ok as-is. > > > >>> > >>>> + && TREE_CODE (bounds) == SSA_NAME > >>> > >>> What about constant bounds? That shouldn't make the call necessary either? > >> > >> Yep, it would be useless. > > > > So please fix. > > > >>> > >>>> + && (bounds_def_stmt = SSA_NAME_DEF_STMT (bounds)) > >>>> + && is_gimple_call (bounds_def_stmt) > >>>> + && gimple_call_fndecl (bounds_def_stmt) == retfndecl > >>>> + && gimple_call_arg (bounds_def_stmt, 0) == ptr)) > >>>> + continue; > >>> > >>> So this all becomes > >>> > >>> if (!gimple_call_with_bounds_p (stmt) > >>> || ((bounds = gimple_call_bndarg (stmt)) > >>> && TREE_CODE (bounds) == SSA_NAME > >>> && (bounds_def_stmt = SSA_NAME_DEF_STMT (bounds)) > >>> && is_gimple_call (bounds_def_stmt) > >>> && gimple_call_chkp_p (bounds_def_stmt, > >>> BUILT_IN_CHKP_BNDRET) > >>> ... > >>> > >>> btw, I wonder how BUILT_IN_CHKP_BNDRET is in scope here but you > >>> need a target hook to compare the fndecl? Why isn't it enough to > >>> just check DECL_FUNCTION_CODE and DECL_BUILT_IN? > >> > >> Call is required because builtins for Pointer Bounds Checker are > >> provided by target depending on available features. That is why > >> gimple_call_builtin_p is not used. I may add new interface function > >> into tree-chkp.h as you propose (e.g. chkp_gimple_call_builtin_p). But > >> I do not see how it may speed-up the code. New function would still > >> need to switch by function code and compare with proper decl which is > >> basically what we have with target hook. > > > > I don't understand - does this mean the builtin calls are in the GIMPLE > > IL even though the target doesn't have the feature? Isn't the presence > > of the call enough? > > Call is generated using function decl provided by target. Therefore we > do not know its code and has to compare using fndecl comparison. > > > > >> It is possible to make faster if use the fact that all chkp builtins > >> codes (normal, not target ones) are consequent. Then we may just check > >> the range and use code as an index in array of chkp fndecls to compare > >> decls. Would it be OK to rely on builtin codes numbering? > > > > Yes, but that's not my point here. In the above code the target hook > > is called all the time, even if !gimple_call_with_bounds_p (). Providing > > a suitable abstraction (or simply relying on DECL_FUNCTION_CODE) > > should fix that. > > Got it. Will fix. > > Thanks, > Ilya > Here is a fixed version. Bootstrapped and tested on linux_x86-64. Thanks, Ilya --- gcc/ 2014-06-05 Ilya Enkovich * tree-ssa-dce.c: Include tree-chkp.h. (propagate_necessity): For free call fed by alloc check bounds are also provided by the same alloc. (eliminate_unnecessary_stmts): Handle BUILT_IN_CHKP_BNDRET used by free calls. diff --git a/gcc/tree-ssa-dce.c b/gcc/tree-ssa-dce.c index 13a71ce..f186706 100644 --- a/gcc/tree-ssa-dce.c +++ b/gcc/tree-ssa-dce.c @@ -73,6 +73,7 @@ along with GCC; see the file COPYING3. If not see #include "flags.h" #include "cfgloop.h" #include "tree-scalar-evolution.h" +#include "tree-chkp.h" static struct stmt_stats { @@ -778,7 +779,22 @@ propagate_necessity (bool aggressive) && DECL_BUILT_IN_CLASS (def_callee) == BUILT_IN_NORMAL && (DECL_FUNCTION_CODE (def_callee) == BUILT_IN_MALLOC || DECL_FUNCTION_CODE (def_callee) == BUILT_IN_CALLOC)) - continue; + { + gimple bounds_def_stmt; + tree bounds; + + /* For instrumented calls we should also check used + bounds are returned by the same allocation call. */ + if (!gimple_call_with_bounds_p (stmt) + || ((bounds = gimple_call_arg (stmt, 1)) + && TREE_CODE (bounds) == SSA_NAME + && (bounds_def_stmt = SSA_NAME_DEF_STMT (bounds)) + && is_gimple_call (bounds_def_stmt) + && chkp_gimple_call_builtin_p (bounds_def_stmt, + BUILT_IN_CHKP_BNDRET) + && gimple_call_arg (bounds_def_stmt, 0) == ptr)) + continue; + } } FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE) @@ -1204,6 +1220,23 @@ eliminate_unnecessary_stmts (void) && !gimple_plf (def_stmt, STMT_NECESSARY)) gimple_set_plf (stmt, STMT_NECESSARY, false); } + /* We did not propagate necessity for free calls fed + by allocation function to allow unnecessary + alloc-free sequence elimination. For instrumented + calls it also means we did not mark bounds producer + as necessary and it is time to do it in case free + call is not removed. */ + if (gimple_call_with_bounds_p (stmt)) + { + gimple bounds_def_stmt; + tree bounds = gimple_call_arg (stmt, 1); + gcc_assert (TREE_CODE (bounds) == SSA_NAME); + bounds_def_stmt = SSA_NAME_DEF_STMT (bounds); + if (bounds_def_stmt + && !gimple_plf (bounds_def_stmt, STMT_NECESSARY)) + gimple_set_plf (bounds_def_stmt, STMT_NECESSARY, + gimple_plf (stmt, STMT_NECESSARY)); + } } /* If GSI is not necessary then remove it. */ @@ -1233,7 +1266,9 @@ eliminate_unnecessary_stmts (void) && DECL_FUNCTION_CODE (call) != BUILT_IN_CALLOC && DECL_FUNCTION_CODE (call) != BUILT_IN_ALLOCA && (DECL_FUNCTION_CODE (call) - != BUILT_IN_ALLOCA_WITH_ALIGN)))) + != BUILT_IN_ALLOCA_WITH_ALIGN))) + /* Avoid doing so for bndret calls for the same reason. */ + && !chkp_gimple_call_builtin_p (stmt, BUILT_IN_CHKP_BNDRET)) { something_changed = true; if (dump_file && (dump_flags & TDF_DETAILS))