Patchwork Fix __builtin_unreachable related regression (PR middle-end/60482)

login
register
mail settings
Submitter Jakub Jelinek
Date March 11, 2014, 3:41 p.m.
Message ID <20140311154105.GC22862@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/329138/
State New
Headers show

Comments

Jakub Jelinek - March 11, 2014, 3:41 p.m.
Hi!

As described in the PR, the r208165 change regressed following test.
The problem is that VRP inserts a useless ASSERT_EXPR right before
__builtin_unreachable () (obviously, the uses of the ASSERT_EXPR
lhs aren't and can't be used by anything), which then prevents
assert_unreachable_fallthru_edge_p from detecting it properly
(but, even ignoring ASSERT_EXPRs there still would fail, because
the ASSERT_EXPR adds another user of the SSA_NAME we check imm uses for).

Perhaps FOUND_IN_SUBGRAPH (4.3 and earlier era) would be always true
here, but live_on_edge provably isn't always true, so it makes sense to test
it, something that isn't live on the edge is useless.

The tree-cfg.c change is just small improvement discovered when looking into
it, clobber stmts before __builtin_unreachable can be certainly ignored,
they don't do anything.

The patch regresses ssa-ifcombine-10.c testcase, I'll post a fix for that
momentarily.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2014-03-11  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/60482
	* tree-vrp.c (register_edge_assert_for_1): Don't add assert
	if there are multiple uses, but op doesn't live on E edge.
	* tree-cfg.c (assert_unreachable_fallthru_edge_p): Also ignore
	clobber stmts before __builtin_unreachable.

	* gcc.dg/vect/pr60482.c: New test.


	Jakub
Richard Guenther - March 12, 2014, 8:50 a.m.
On Tue, 11 Mar 2014, Jakub Jelinek wrote:

> Hi!
> 
> As described in the PR, the r208165 change regressed following test.
> The problem is that VRP inserts a useless ASSERT_EXPR right before
> __builtin_unreachable () (obviously, the uses of the ASSERT_EXPR
> lhs aren't and can't be used by anything), which then prevents
> assert_unreachable_fallthru_edge_p from detecting it properly
> (but, even ignoring ASSERT_EXPRs there still would fail, because
> the ASSERT_EXPR adds another user of the SSA_NAME we check imm uses for).
> 
> Perhaps FOUND_IN_SUBGRAPH (4.3 and earlier era) would be always true
> here, but live_on_edge provably isn't always true, so it makes sense to test
> it, something that isn't live on the edge is useless.
> 
> The tree-cfg.c change is just small improvement discovered when looking into
> it, clobber stmts before __builtin_unreachable can be certainly ignored,
> they don't do anything.
> 
> The patch regresses ssa-ifcombine-10.c testcase, I'll post a fix for that
> momentarily.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2014-03-11  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/60482
> 	* tree-vrp.c (register_edge_assert_for_1): Don't add assert
> 	if there are multiple uses, but op doesn't live on E edge.
> 	* tree-cfg.c (assert_unreachable_fallthru_edge_p): Also ignore
> 	clobber stmts before __builtin_unreachable.
> 
> 	* gcc.dg/vect/pr60482.c: New test.
> 
> --- gcc/tree-vrp.c.jj	2014-01-25 00:11:37.000000000 +0100
> +++ gcc/tree-vrp.c	2014-03-10 14:59:03.748267354 +0100
> @@ -5423,12 +5423,9 @@ register_edge_assert_for_1 (tree op, enu
>      return false;
>  
>    /* We know that OP will have a zero or nonzero value.  If OP is used
> -     more than once go ahead and register an assert for OP.
> -
> -     The FOUND_IN_SUBGRAPH support is not helpful in this situation as
> -     it will always be set for OP (because OP is used in a COND_EXPR in
> -     the subgraph).  */
> -  if (!has_single_use (op))
> +     more than once go ahead and register an assert for OP.  */
> +  if (live_on_edge (e, op)
> +      && !has_single_use (op))
>      {
>        val = build_int_cst (TREE_TYPE (op), 0);
>        register_new_assert_for (op, op, code, val, NULL, e, bsi);
> --- gcc/tree-cfg.c.jj	2014-02-20 21:38:42.000000000 +0100
> +++ gcc/tree-cfg.c	2014-03-10 14:59:52.058957446 +0100
> @@ -410,9 +410,9 @@ assert_unreachable_fallthru_edge_p (edge
>  	  if (gsi_end_p (gsi))
>  	    return false;
>  	  stmt = gsi_stmt (gsi);
> -	  if (is_gimple_debug (stmt))
> +	  while (is_gimple_debug (stmt) || gimple_clobber_p (stmt))
>  	    {
> -	      gsi_next_nondebug (&gsi);
> +	      gsi_next (&gsi);
>  	      if (gsi_end_p (gsi))
>  		return false;
>  	      stmt = gsi_stmt (gsi);
> --- gcc/testsuite/gcc.dg/vect/pr60482.c.jj	2014-03-10 15:08:16.700085976 +0100
> +++ gcc/testsuite/gcc.dg/vect/pr60482.c	2014-03-10 15:15:09.609738455 +0100
> @@ -0,0 +1,20 @@
> +/* PR middle-end/60482 */
> +/* { dg-do compile } */
> +/* { dg-additional-options "-Ofast" } */
> +/* { dg-require-effective-target vect_int } */
> +
> +double
> +foo (double *x, int n)
> +{
> +  double p = 0.0;
> +  int i;
> +  x = __builtin_assume_aligned (x, 128);
> +  if (n % 128)
> +    __builtin_unreachable ();
> +  for (i = 0; i < n; i++)
> +    p += x[i];
> +  return p;
> +}
> +
> +/* { dg-final { scan-tree-dump-not "epilog loop required" "vect" } } */
> +/* { dg-final { cleanup-tree-dump "vect" } } */
> 
> 	Jakub
> 
>

Patch

--- gcc/tree-vrp.c.jj	2014-01-25 00:11:37.000000000 +0100
+++ gcc/tree-vrp.c	2014-03-10 14:59:03.748267354 +0100
@@ -5423,12 +5423,9 @@  register_edge_assert_for_1 (tree op, enu
     return false;
 
   /* We know that OP will have a zero or nonzero value.  If OP is used
-     more than once go ahead and register an assert for OP.
-
-     The FOUND_IN_SUBGRAPH support is not helpful in this situation as
-     it will always be set for OP (because OP is used in a COND_EXPR in
-     the subgraph).  */
-  if (!has_single_use (op))
+     more than once go ahead and register an assert for OP.  */
+  if (live_on_edge (e, op)
+      && !has_single_use (op))
     {
       val = build_int_cst (TREE_TYPE (op), 0);
       register_new_assert_for (op, op, code, val, NULL, e, bsi);
--- gcc/tree-cfg.c.jj	2014-02-20 21:38:42.000000000 +0100
+++ gcc/tree-cfg.c	2014-03-10 14:59:52.058957446 +0100
@@ -410,9 +410,9 @@  assert_unreachable_fallthru_edge_p (edge
 	  if (gsi_end_p (gsi))
 	    return false;
 	  stmt = gsi_stmt (gsi);
-	  if (is_gimple_debug (stmt))
+	  while (is_gimple_debug (stmt) || gimple_clobber_p (stmt))
 	    {
-	      gsi_next_nondebug (&gsi);
+	      gsi_next (&gsi);
 	      if (gsi_end_p (gsi))
 		return false;
 	      stmt = gsi_stmt (gsi);
--- gcc/testsuite/gcc.dg/vect/pr60482.c.jj	2014-03-10 15:08:16.700085976 +0100
+++ gcc/testsuite/gcc.dg/vect/pr60482.c	2014-03-10 15:15:09.609738455 +0100
@@ -0,0 +1,20 @@ 
+/* PR middle-end/60482 */
+/* { dg-do compile } */
+/* { dg-additional-options "-Ofast" } */
+/* { dg-require-effective-target vect_int } */
+
+double
+foo (double *x, int n)
+{
+  double p = 0.0;
+  int i;
+  x = __builtin_assume_aligned (x, 128);
+  if (n % 128)
+    __builtin_unreachable ();
+  for (i = 0; i < n; i++)
+    p += x[i];
+  return p;
+}
+
+/* { dg-final { scan-tree-dump-not "epilog loop required" "vect" } } */
+/* { dg-final { cleanup-tree-dump "vect" } } */