Fix fold-const (X & C) ? C : 0 optimization (PR middle-end/87248)

Message ID 20180912075951.GD8250@tucnak
State New
Headers show
Series
  • Fix fold-const (X & C) ? C : 0 optimization (PR middle-end/87248)
Related show

Commit Message

Jakub Jelinek Sept. 12, 2018, 7:59 a.m.
Hi!

The following testcase is miscompiled, because it optimizes
(x & -128) ? -128 : 0 to (x & -128) when it shouldn't.  The problem is if
the type of the COND_EXPR has smaller precision than the BIT_AND_EXPR in the
test, we verify that integer_pow2p (arg1), which is true in this case (-128
in signed char is a power of two), and then operand_equal_p between that
value and the constant in BIT_AND_EXPR's second argument (operand_equal_p
compares only the value, and -128 == -128).  The following patch verifies
also that the BIT_AND_EXPR's second operand is a power of two.

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

2018-09-12  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/87248
	* fold-const.c (fold_ternary_loc) <case COND_EXPR>: Verify also that
	BIT_AND_EXPR's second operand is a power of two.  Formatting fix.

	* c-c++-common/torture/pr87248.c: New test.


	Jakub

Comments

Richard Biener Sept. 12, 2018, 8:02 a.m. | #1
On Wed, 12 Sep 2018, Jakub Jelinek wrote:

> Hi!

> 

> The following testcase is miscompiled, because it optimizes

> (x & -128) ? -128 : 0 to (x & -128) when it shouldn't.  The problem is if

> the type of the COND_EXPR has smaller precision than the BIT_AND_EXPR in the

> test, we verify that integer_pow2p (arg1), which is true in this case (-128

> in signed char is a power of two), and then operand_equal_p between that

> value and the constant in BIT_AND_EXPR's second argument (operand_equal_p

> compares only the value, and -128 == -128).  The following patch verifies

> also that the BIT_AND_EXPR's second operand is a power of two.

> 

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

> release branches?


OK.

Richard.

> 

> 2018-09-12  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR middle-end/87248

> 	* fold-const.c (fold_ternary_loc) <case COND_EXPR>: Verify also that

> 	BIT_AND_EXPR's second operand is a power of two.  Formatting fix.

> 

> 	* c-c++-common/torture/pr87248.c: New test.

> 

> --- gcc/fold-const.c.jj	2018-09-06 09:41:59.000000000 +0200

> +++ gcc/fold-const.c	2018-09-08 00:12:28.332418784 +0200

> @@ -11607,10 +11607,16 @@ fold_ternary_loc (location_t loc, enum t

>  	  && integer_pow2p (arg1)

>  	  && TREE_CODE (TREE_OPERAND (arg0, 0)) == BIT_AND_EXPR

>  	  && operand_equal_p (TREE_OPERAND (TREE_OPERAND (arg0, 0), 1),

> -			      arg1, OEP_ONLY_CONST))

> +			      arg1, OEP_ONLY_CONST)

> +	  /* operand_equal_p compares just value, not precision, so e.g.

> +	     arg1 could be 8-bit -128 and be power of two, but BIT_AND_EXPR

> +	     second operand 32-bit -128, which is not a power of two (or vice

> +	     versa.  */

> +	  && integer_pow2p (TREE_OPERAND (TREE_OPERAND (arg0, 0), 1)))

>  	return pedantic_non_lvalue_loc (loc,

> -				    fold_convert_loc (loc, type,

> -						      TREE_OPERAND (arg0, 0)));

> +					fold_convert_loc (loc, type,

> +							  TREE_OPERAND (arg0,

> +									0)));

>  

>        /* Disable the transformations below for vectors, since

>  	 fold_binary_op_with_conditional_arg may undo them immediately,

> --- gcc/testsuite/c-c++-common/torture/pr87248.c.jj	2018-09-08 01:13:43.431334239 +0200

> +++ gcc/testsuite/c-c++-common/torture/pr87248.c	2018-09-08 01:14:55.446593520 +0200

> @@ -0,0 +1,36 @@

> +/* PR middle-end/87248 */

> +/* { dg-do run } */

> +

> +void

> +foo (signed char *p, int q)

> +{

> +  *p = q & (-__SCHAR_MAX__ - 1) ? (-__SCHAR_MAX__ - 1) : 0;

> +}

> +

> +int

> +bar (long long x)

> +{

> +  return x & (-__INT_MAX__ - 1) ? (-__INT_MAX__ - 1) : 0;

> +}

> +

> +int

> +main ()

> +{

> +#if __INT_MAX__ > 4 * __SCHAR_MAX__

> +  signed char a[4];

> +  foo (a, __SCHAR_MAX__ + 1U);

> +  foo (a + 1, 2 * (__SCHAR_MAX__ + 1U));

> +  foo (a + 2, -__INT_MAX__ - 1);

> +  foo (a + 3, (__SCHAR_MAX__ + 1U) / 2);

> +  if (a[0] != (-__SCHAR_MAX__ - 1) || a[1] != a[0] || a[2] != a[0] || a[3] != 0)

> +    __builtin_abort ();

> +#endif

> +#if __LONG_LONG_MAX__ > 4 * __INT_MAX__

> +  if (bar (__INT_MAX__ + 1LL) != (-__INT_MAX__ - 1)

> +      || bar (2 * (__INT_MAX__ + 1LL)) != (-__INT_MAX__ - 1)

> +      || bar (-__LONG_LONG_MAX__ - 1) != (-__INT_MAX__ - 1)

> +      || bar ((__INT_MAX__ + 1LL) / 2) != 0)

> +    __builtin_abort ();

> +#endif

> +  return 0;

> +}

> 

> 	Jakub

> 

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Patch

--- gcc/fold-const.c.jj	2018-09-06 09:41:59.000000000 +0200
+++ gcc/fold-const.c	2018-09-08 00:12:28.332418784 +0200
@@ -11607,10 +11607,16 @@  fold_ternary_loc (location_t loc, enum t
 	  && integer_pow2p (arg1)
 	  && TREE_CODE (TREE_OPERAND (arg0, 0)) == BIT_AND_EXPR
 	  && operand_equal_p (TREE_OPERAND (TREE_OPERAND (arg0, 0), 1),
-			      arg1, OEP_ONLY_CONST))
+			      arg1, OEP_ONLY_CONST)
+	  /* operand_equal_p compares just value, not precision, so e.g.
+	     arg1 could be 8-bit -128 and be power of two, but BIT_AND_EXPR
+	     second operand 32-bit -128, which is not a power of two (or vice
+	     versa.  */
+	  && integer_pow2p (TREE_OPERAND (TREE_OPERAND (arg0, 0), 1)))
 	return pedantic_non_lvalue_loc (loc,
-				    fold_convert_loc (loc, type,
-						      TREE_OPERAND (arg0, 0)));
+					fold_convert_loc (loc, type,
+							  TREE_OPERAND (arg0,
+									0)));
 
       /* Disable the transformations below for vectors, since
 	 fold_binary_op_with_conditional_arg may undo them immediately,
--- gcc/testsuite/c-c++-common/torture/pr87248.c.jj	2018-09-08 01:13:43.431334239 +0200
+++ gcc/testsuite/c-c++-common/torture/pr87248.c	2018-09-08 01:14:55.446593520 +0200
@@ -0,0 +1,36 @@ 
+/* PR middle-end/87248 */
+/* { dg-do run } */
+
+void
+foo (signed char *p, int q)
+{
+  *p = q & (-__SCHAR_MAX__ - 1) ? (-__SCHAR_MAX__ - 1) : 0;
+}
+
+int
+bar (long long x)
+{
+  return x & (-__INT_MAX__ - 1) ? (-__INT_MAX__ - 1) : 0;
+}
+
+int
+main ()
+{
+#if __INT_MAX__ > 4 * __SCHAR_MAX__
+  signed char a[4];
+  foo (a, __SCHAR_MAX__ + 1U);
+  foo (a + 1, 2 * (__SCHAR_MAX__ + 1U));
+  foo (a + 2, -__INT_MAX__ - 1);
+  foo (a + 3, (__SCHAR_MAX__ + 1U) / 2);
+  if (a[0] != (-__SCHAR_MAX__ - 1) || a[1] != a[0] || a[2] != a[0] || a[3] != 0)
+    __builtin_abort ();
+#endif
+#if __LONG_LONG_MAX__ > 4 * __INT_MAX__
+  if (bar (__INT_MAX__ + 1LL) != (-__INT_MAX__ - 1)
+      || bar (2 * (__INT_MAX__ + 1LL)) != (-__INT_MAX__ - 1)
+      || bar (-__LONG_LONG_MAX__ - 1) != (-__INT_MAX__ - 1)
+      || bar ((__INT_MAX__ + 1LL) / 2) != 0)
+    __builtin_abort ();
+#endif
+  return 0;
+}