From: Tom de Vries <Tom_deVries@mentor.com>
To: Jakub Jelinek <jakub@redhat.com>
Cc: Jeff Law <law@redhat.com>,
Richard Biener <richard.guenther@gmail.com>,
mliska <mliska@suse.cz>, GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] Skip ubsan internal fns in tail-merge
Date: Mon, 07 Mar 2016 11:56:00 -0000 [thread overview]
Message-ID: <56DD6C74.8060706@mentor.com> (raw)
In-Reply-To: <20160307113236.GT3017@tucnak.redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1714 bytes --]
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 <tom@codesourcery.com>
>>
>> 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
[-- Attachment #2: 0001-Skip-ubsan-asan-internal-fns-with-different-location-in-tail-merge.patch --]
[-- Type: text/x-patch, Size: 2534 bytes --]
Skip ubsan/asan internal fns with different location in tail-merge
2016-03-07 Tom de Vries <tom@codesourcery.com>
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);
next prev parent reply other threads:[~2016-03-07 11:56 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-09 14:10 [RFC, PATCH 0/3] tree-ssa-tail-merge engine replacement mliska
2015-07-09 14:10 ` [PATCH 2/3] Fully remove legacy tree-ssa-tail-merge value numbering infrastructure mliska
2015-07-09 15:38 ` Jeff Law
2015-07-10 9:31 ` Richard Biener
2015-07-09 14:10 ` [PATCH 1/3] tree-ssa-tail-merge: add IPA ICF infrastructure mliska
2015-07-09 16:23 ` Jeff Law
2015-07-16 11:25 ` Martin Liška
2015-07-20 7:53 ` Martin Liška
2015-08-03 15:18 ` Martin Liška
2015-07-20 8:24 ` Martin Liška
2015-08-03 17:41 ` Jeff Law
2015-08-03 17:38 ` Jeff Law
2015-08-05 15:16 ` Martin Liška
2015-08-19 17:27 ` Jeff Law
2015-07-09 14:10 ` [PATCH 3/3] Fix ubsan tests by disabling of an optimization mliska
2015-07-09 14:13 ` Jakub Jelinek
2015-07-09 15:34 ` Jeff Law
2015-07-09 15:42 ` Jakub Jelinek
2015-07-09 16:37 ` Jeff Law
2015-07-10 8:19 ` Richard Biener
2015-07-10 20:11 ` Jeff Law
2016-03-07 9:49 ` [PATCH] Skip ubsan internal fns in tail-merge Tom de Vries
2016-03-07 10:02 ` Jakub Jelinek
2016-03-07 11:26 ` Tom de Vries
2016-03-07 11:32 ` Jakub Jelinek
2016-03-07 11:56 ` Tom de Vries [this message]
2015-07-09 15:29 ` [RFC, PATCH 0/3] tree-ssa-tail-merge engine replacement Jeff Law
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56DD6C74.8060706@mentor.com \
--to=tom_devries@mentor.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=law@redhat.com \
--cc=mliska@suse.cz \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).