diff mbox

Skip ubsan internal fns in tail-merge

Message ID 56DD6552.1030105@mentor.com
State New
Headers show

Commit Message

Tom de Vries March 7, 2016, 11:26 a.m. UTC 
On 07/03/16 11:01, Jakub Jelinek wrote:
> On Mon, Mar 07, 2016 at 10:49:14AM +0100, Tom de Vries wrote:
>> This patch implements that.
>>
>> Should the asan internal function (ASAN_CHECK) be handled in the same way,
>> as suggested here ( https://gcc.gnu.org/ml/gcc-patches/2015-07/msg00776.html
>> )?
>>
>> OK for stage4 trunk if bootstrap and reg-test succeeds?
>
> Please certainly add ASAN_CHECK to the list.

Done.

> Perhaps more precise would be to allow merging them, but only if
> gimple_location (stmt1) == gimple_location (stmt2).
>

Done.

> Also, I wonder why you need to check both stmts individually, wouldn't it be
> better (both for the is_tm_ending and the ubsan/asan internal calls) to
> first call gimple_equal_p and only if it says the two are equal, check
> is_tm_ending (only on one stmt, if they are equal, then I hope is_tm_ending
> is necessarily equal for both), and similarly for the ubsan/asan ifns
> compare the gimple_location?
>

Done.

OK for stage4 trunk if bootstrap and reg-test succeeds?

Thanks,
- Tom

Comments

Jakub Jelinek March 7, 2016, 11:32 a.m. UTC | #1 
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.

> +	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);

	Jakub
diff mbox

Patch

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, 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.

---
 gcc/tree-ssa-tail-merge.c | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
index 5d32790..13ea4fe 100644
--- a/gcc/tree-ssa-tail-merge.c
+++ b/gcc/tree-ssa-tail-merge.c
@@ -1207,6 +1207,39 @@  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:
+	  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 +1259,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);