diff mbox

Don't simplify_conversion_from_bitmask on x & 1 (PR tree-optimization/59014)

Message ID 20131107151511.GK30062@redhat.com
State New
Headers show

Commit Message

Marek Polacek Nov. 7, 2013, 3:15 p.m. UTC 
Here, forward propagation turned
  _6 = a.1_5 & 1;
  _7 = (_Bool) _6;
into
  _7 = (_Bool) a.1_5;
but that's wrong: e.g. if a = 2 then (_Bool) _6 is 0, but (_Bool) a.1_5 is 1.
If a is an odd number, this is correct, but we don't know the value
of a, so I disabled this optimization for "x & 1" cases.  For e.g. "x & 255",
this still works correctly.

In this PR, VRP created wrong ASSERT_EXPR because of this, thus we were
miscompiling the following testcase...

Regtested/bootstrapped on x86_64-linux, ok for trunk?

2013-11-07  Marek Polacek  <polacek@redhat.com>

	PR tree-optimization/59014
	* tree-ssa-forwprop.c (simplify_conversion_from_bitmask): Don't
	optimize x & 1 expressions here.
testsuite/
	* gcc.dg/torture/pr59014.c: New test.


	Marek

Comments

Jakub Jelinek Nov. 7, 2013, 3:26 p.m. UTC | #1 
On Thu, Nov 07, 2013 at 04:15:11PM +0100, Marek Polacek wrote:
> Here, forward propagation turned
>   _6 = a.1_5 & 1;
>   _7 = (_Bool) _6;
> into
>   _7 = (_Bool) a.1_5;
> but that's wrong: e.g. if a = 2 then (_Bool) _6 is 0, but (_Bool) a.1_5 is 1.
> If a is an odd number, this is correct, but we don't know the value
> of a, so I disabled this optimization for "x & 1" cases.  For e.g. "x & 255",
> this still works correctly.
> 
> In this PR, VRP created wrong ASSERT_EXPR because of this, thus we were
> miscompiling the following testcase...
> 
> Regtested/bootstrapped on x86_64-linux, ok for trunk?

Isn't the problem here not the constant 1, but the fact that TREE_CODE
(lhs_type) == BOOLEAN_TYPE, with the special properties of boolean types?
I mean, if lhs_type is say some normal INTEGER_TYPE with precision 1
(or say 3 and stmt & 3 etc.), then the cast alone should cover the required
masking.

> 2013-11-07  Marek Polacek  <polacek@redhat.com>
> 
> 	PR tree-optimization/59014
> 	* tree-ssa-forwprop.c (simplify_conversion_from_bitmask): Don't
> 	optimize x & 1 expressions here.
> testsuite/
> 	* gcc.dg/torture/pr59014.c: New test.
> 
> --- gcc/tree-ssa-forwprop.c.mp	2013-11-07 14:30:43.785733810 +0100
> +++ gcc/tree-ssa-forwprop.c	2013-11-07 14:42:31.836765375 +0100
> @@ -1199,9 +1199,16 @@ simplify_conversion_from_bitmask (gimple
>        if (TREE_CODE (rhs_def_operand1) == SSA_NAME
>  	  && ! SSA_NAME_OCCURS_IN_ABNORMAL_PHI (rhs_def_operand1)
>  	  && TREE_CODE (rhs_def_operand2) == INTEGER_CST
> +	  /* We can't turn
> +	       _6 = a & 1;
> +	       _7 = (_Bool) _6;
> +	     into
> +	       _7 = (_Bool) a;
> +	     as that's wrong if A is an even number.  */
> +	  && ! integer_onep (rhs_def_operand2)
>  	  && operand_equal_p (rhs_def_operand2,
>  			      build_low_bits_mask (TREE_TYPE (rhs_def_operand2),
> -			       			   TYPE_PRECISION (lhs_type)),
> +						   TYPE_PRECISION (lhs_type)),
>  						   0))
>  	{
>  	  /* This is an optimizable case.  Replace the source operand
> 
> 	Marek

	Jakub
Richard Biener Nov. 7, 2013, 3:39 p.m. UTC | #2 
On Thu, Nov 7, 2013 at 4:15 PM, Marek Polacek <polacek@redhat.com> wrote:
> Here, forward propagation turned
>   _6 = a.1_5 & 1;
>   _7 = (_Bool) _6;
> into
>   _7 = (_Bool) a.1_5;
> but that's wrong: e.g. if a = 2 then (_Bool) _6 is 0, but (_Bool) a.1_5 is 1.

?

(_Bool) 2

should truncate the value to zero.

Richard.

> If a is an odd number, this is correct, but we don't know the value
> of a, so I disabled this optimization for "x & 1" cases.  For e.g. "x & 255",
> this still works correctly.


> In this PR, VRP created wrong ASSERT_EXPR because of this, thus we were
> miscompiling the following testcase...
>
> Regtested/bootstrapped on x86_64-linux, ok for trunk?
>
> 2013-11-07  Marek Polacek  <polacek@redhat.com>
>
>         PR tree-optimization/59014
>         * tree-ssa-forwprop.c (simplify_conversion_from_bitmask): Don't
>         optimize x & 1 expressions here.
> testsuite/
>         * gcc.dg/torture/pr59014.c: New test.
>
> --- gcc/testsuite/gcc.dg/torture/pr59014.c.mp   2013-11-07 14:47:38.040941144 +0100
> +++ gcc/testsuite/gcc.dg/torture/pr59014.c      2013-11-07 14:49:03.646271031 +0100
> @@ -0,0 +1,26 @@
> +/* PR tree-optimization/59014 */
> +/* { dg-do run } */
> +
> +int a = 2, b, c, d;
> +
> +int
> +foo ()
> +{
> +  for (;; c++)
> +    if ((b > 0) | (a & 1))
> +      ;
> +    else
> +      {
> +       d = a;
> +       return 0;
> +      }
> +}
> +
> +int
> +main ()
> +{
> +  foo ();
> +  if (d != 2)
> +    __builtin_abort ();
> +  return 0;
> +}
> --- gcc/tree-ssa-forwprop.c.mp  2013-11-07 14:30:43.785733810 +0100
> +++ gcc/tree-ssa-forwprop.c     2013-11-07 14:42:31.836765375 +0100
> @@ -1199,9 +1199,16 @@ simplify_conversion_from_bitmask (gimple
>        if (TREE_CODE (rhs_def_operand1) == SSA_NAME
>           && ! SSA_NAME_OCCURS_IN_ABNORMAL_PHI (rhs_def_operand1)
>           && TREE_CODE (rhs_def_operand2) == INTEGER_CST
> +         /* We can't turn
> +              _6 = a & 1;
> +              _7 = (_Bool) _6;
> +            into
> +              _7 = (_Bool) a;
> +            as that's wrong if A is an even number.  */
> +         && ! integer_onep (rhs_def_operand2)
>           && operand_equal_p (rhs_def_operand2,
>                               build_low_bits_mask (TREE_TYPE (rhs_def_operand2),
> -                                                  TYPE_PRECISION (lhs_type)),
> +                                                  TYPE_PRECISION (lhs_type)),
>                                                    0))
>         {
>           /* This is an optimizable case.  Replace the source operand
>
>         Marek
Jakub Jelinek Nov. 7, 2013, 3:48 p.m. UTC | #3 
On Thu, Nov 07, 2013 at 04:39:03PM +0100, Richard Biener wrote:
> On Thu, Nov 7, 2013 at 4:15 PM, Marek Polacek <polacek@redhat.com> wrote:
> > Here, forward propagation turned
> >   _6 = a.1_5 & 1;
> >   _7 = (_Bool) _6;
> > into
> >   _7 = (_Bool) a.1_5;
> > but that's wrong: e.g. if a = 2 then (_Bool) _6 is 0, but (_Bool) a.1_5 is 1.
> 
> ?
> 
> (_Bool) 2
> 
> should truncate the value to zero.

You're right, say:
_Bool foo (int x)
{
  return (_Bool) (x & 1);
}
which due to the simplify_conversion_from_bitmask optimization is optimized
into:
  _Bool _3;

  <bb 2>:
  _3 = (_Bool) x_1(D);
  return _3;
is indeed expanded as:
            (set (reg:QI 90 [ D.1789 ])
                (and:QI (reg:QI 90 [ D.1789 ])
                    (const_int 1 [0x1])))

	Jakub
Marek Polacek Nov. 7, 2013, 4:37 p.m. UTC | #4 
On Thu, Nov 07, 2013 at 04:39:03PM +0100, Richard Biener wrote:
> On Thu, Nov 7, 2013 at 4:15 PM, Marek Polacek <polacek@redhat.com> wrote:
> > Here, forward propagation turned
> >   _6 = a.1_5 & 1;
> >   _7 = (_Bool) _6;
> > into
> >   _7 = (_Bool) a.1_5;
> > but that's wrong: e.g. if a = 2 then (_Bool) _6 is 0, but (_Bool) a.1_5 is 1.
> 
> ?
> 
> (_Bool) 2
> 
> should truncate the value to zero.

Aha.  I can see now, said the blind man.

Anyway, I'll revisit this in stage 3; I now suspect the special
handling of BIT_IOR_EXPR in register_edge_assert_for_1.

	Marek
diff mbox

Patch

--- gcc/testsuite/gcc.dg/torture/pr59014.c.mp	2013-11-07 14:47:38.040941144 +0100
+++ gcc/testsuite/gcc.dg/torture/pr59014.c	2013-11-07 14:49:03.646271031 +0100
@@ -0,0 +1,26 @@ 
+/* PR tree-optimization/59014 */
+/* { dg-do run } */
+
+int a = 2, b, c, d;
+
+int
+foo ()
+{
+  for (;; c++)
+    if ((b > 0) | (a & 1))
+      ;
+    else
+      {
+	d = a;
+	return 0;
+      }
+}
+
+int
+main ()
+{
+  foo ();
+  if (d != 2)
+    __builtin_abort ();
+  return 0;
+}
--- gcc/tree-ssa-forwprop.c.mp	2013-11-07 14:30:43.785733810 +0100
+++ gcc/tree-ssa-forwprop.c	2013-11-07 14:42:31.836765375 +0100
@@ -1199,9 +1199,16 @@  simplify_conversion_from_bitmask (gimple
       if (TREE_CODE (rhs_def_operand1) == SSA_NAME
 	  && ! SSA_NAME_OCCURS_IN_ABNORMAL_PHI (rhs_def_operand1)
 	  && TREE_CODE (rhs_def_operand2) == INTEGER_CST
+	  /* We can't turn
+	       _6 = a & 1;
+	       _7 = (_Bool) _6;
+	     into
+	       _7 = (_Bool) a;
+	     as that's wrong if A is an even number.  */
+	  && ! integer_onep (rhs_def_operand2)
 	  && operand_equal_p (rhs_def_operand2,
 			      build_low_bits_mask (TREE_TYPE (rhs_def_operand2),
-			       			   TYPE_PRECISION (lhs_type)),
+						   TYPE_PRECISION (lhs_type)),
 						   0))
 	{
 	  /* This is an optimizable case.  Replace the source operand