From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 118823 invoked by alias); 7 Mar 2016 11:56:50 -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 118810 invoked by uid 89); 7 Mar 2016 11:56:49 -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,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 07 Mar 2016 11:56:48 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-FEM-01.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1actmF-0000UG-5Y from Tom_deVries@mentor.com ; Mon, 07 Mar 2016 03:56:43 -0800 Received: from [127.0.0.1] (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.3.224.2; Mon, 7 Mar 2016 11:56:41 +0000 Subject: Re: [PATCH] Skip ubsan internal fns in tail-merge To: Jakub Jelinek References: <1c507aae804089fe39102391ff554d51a20ba391.1436450591.git.mliska@suse.cz> <20150709141312.GS10247@tucnak.redhat.com> <559E9481.70505@redhat.com> <20150709154153.GV10247@tucnak.redhat.com> <559EA345.6070105@redhat.com> <55A02707.6000505@redhat.com> <56DD4E9A.3030009@mentor.com> <20160307100155.GS3017@tucnak.redhat.com> <56DD6552.1030105@mentor.com> <20160307113236.GT3017@tucnak.redhat.com> CC: Jeff Law , Richard Biener , mliska , GCC Patches From: Tom de Vries Message-ID: <56DD6C74.8060706@mentor.com> Date: Mon, 07 Mar 2016 11:56:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160307113236.GT3017@tucnak.redhat.com> Content-Type: multipart/mixed; boundary="------------010809020609070906020901" X-SW-Source: 2016-03/txt/msg00471.txt.bz2 --------------010809020609070906020901 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Content-length: 1714 On 07/03/16 12:32, Jakub Jelinek wrote: > On Mon, Mar 07, 2016 at 12:26:10PM +0100, Tom de Vries wrote: >> OK for stage4 trunk if bootstrap and reg-test succeeds? > > Ok, with a minor nits. > >> 2016-03-07 Tom de Vries >> >> PR tree-optimization/70116 >> * tree-ssa-tail-merge.c (merge_stmts_p): New function, factored out >> of ... >> (find_duplicate): ... here. Move merge_stmts_p test to after >> gimple_equal_p test. >> (merge_stmts_p): Return false for ubsan/asan internal functions with >> different gimple_location. > > Listing the same function twice is weird, and for a single line short test > copied from elsewhere, especially if it originally was used twice and now > just once, I think it is not worth mentioning. I'd just write: > (merge_stmts_p): New function. > (find_duplicate): Use it. Don't test is_tm_ending here. > The only problem I have with that is that the ChangeLog entry doesn't mention ubsan/usan internal fns. I've resolved that by elaborating on the new function. >> + case IFN_UBSAN_NULL: >> + case IFN_UBSAN_BOUNDS: >> + case IFN_UBSAN_VPTR: >> + case IFN_UBSAN_CHECK_ADD: >> + case IFN_UBSAN_CHECK_SUB: >> + case IFN_UBSAN_CHECK_MUL: >> + case IFN_UBSAN_OBJECT_SIZE: >> + case IFN_ASAN_CHECK: > > This deserves a comment, you should mention what has been mentioned in the > thread, that for these internal functions gimple_location is like another > artificial parameter, or explain what it would mean if it attempted to merge > these stmts with different locations. >> + return gimple_location (stmt1) == gimple_location (stmt2); Done. [ And I fixed a missing closing brace error. ] Currently bootstrapping/reg-testing. Thanks, - Tom --------------010809020609070906020901 Content-Type: text/x-patch; name= "0001-Skip-ubsan-asan-internal-fns-with-different-location-in-tail-merge.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename*0="0001-Skip-ubsan-asan-internal-fns-with-different-location-in"; filename*1="-tail-merge.patch" Content-length: 2534 Skip ubsan/asan internal fns with different location in tail-merge 2016-03-07 Tom de Vries PR tree-optimization/70116 * tree-ssa-tail-merge.c (merge_stmts_p): New function, handling is_tm_ending stmts and ubsan/asan internal functions. (find_duplicate): Use it. Don't test is_tm_ending here. --- gcc/tree-ssa-tail-merge.c | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-) diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c index 5d32790..e95879f 100644 --- a/gcc/tree-ssa-tail-merge.c +++ b/gcc/tree-ssa-tail-merge.c @@ -1207,6 +1207,42 @@ gsi_advance_bw_nondebug_nonlocal (gimple_stmt_iterator *gsi, tree *vuse, } } +/* Return true if equal (in the sense of gimple_equal_p) statements STMT1 and + STMT2 are allowed to be merged. */ + +static bool +merge_stmts_p (gimple *stmt1, gimple *stmt2) +{ + /* What could be better than this here is to blacklist the bb + containing the stmt, when encountering the stmt f.i. in + same_succ_hash. */ + if (is_tm_ending (stmt1)) + return false; + + if (is_gimple_call (stmt1) + && gimple_call_internal_p (stmt1)) + switch (gimple_call_internal_fn (stmt1)) + { + case IFN_UBSAN_NULL: + case IFN_UBSAN_BOUNDS: + case IFN_UBSAN_VPTR: + case IFN_UBSAN_CHECK_ADD: + case IFN_UBSAN_CHECK_SUB: + case IFN_UBSAN_CHECK_MUL: + case IFN_UBSAN_OBJECT_SIZE: + case IFN_ASAN_CHECK: + /* For these internal functions, gimple_location is an implicit + parameter, which will be used explicitly after expansion. + Merging these statements may cause confusing line numbers in + sanitizer messages. */ + return gimple_location (stmt1) == gimple_location (stmt2); + default: + break; + } + + return true; +} + /* Determines whether BB1 and BB2 (members of same_succ) are duplicates. If so, clusters them. */ @@ -1226,14 +1262,10 @@ find_duplicate (same_succ *same_succ, basic_block bb1, basic_block bb2) gimple *stmt1 = gsi_stmt (gsi1); gimple *stmt2 = gsi_stmt (gsi2); - /* What could be better than this here is to blacklist the bb - containing the stmt, when encountering the stmt f.i. in - same_succ_hash. */ - if (is_tm_ending (stmt1) - || is_tm_ending (stmt2)) + if (!gimple_equal_p (same_succ, stmt1, stmt2)) return; - if (!gimple_equal_p (same_succ, stmt1, stmt2)) + if (!merge_stmts_p (stmt1, stmt2)) return; gsi_prev_nondebug (&gsi1); --------------010809020609070906020901--