Message ID | 20131107151511.GK30062@redhat.com |
---|---|
State | New |
Headers | show |
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
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
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
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
--- 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