Re: [4.3] fix c/39902 for decimal float constant folding

[Janis Johnson <janis187 at us dot ibm dot com>]

On Thu, 2009-07-16 at 10:49 +0200, Richard Guenther wrote:
> On Thu, Jul 16, 2009 at 1:27 AM, Janis Johnson<janis187@us.ibm.com> wrote:
> > On Wed, 2009-07-15 at 23:21 +0200, Richard Guenther wrote:
> >> On Wed, Jul 15, 2009 at 11:14 PM, Janis Johnson<janis187@us.ibm.com> wrote:
> >> > In http://gcc.gnu.org/ml/gcc-patches/2009-06/msg02044.html, Richard
> >> > Guenther approved a version of this patch for trunk and the branches.
> >> > The mainline patch works for the 4.4 branch but not 4.3, where there
> >> > are additional passes that do constant folding.  This patch disables
> >> > RTL constant folding for binary operations involving decimal float
> >> > constants.
> >>
> >> I'd rather have the trunk bits that fixed it there backported than putting
> >> a check on the branch that is not on trunk.  Why doesn't trunk need the
> >> simplify-rtx.c hunk?
> >>
> >> Richard.
> >
> > The changes to tree.c are the same as in the trunk.  In the 4.3
> > branch the call to simplify_binary_operation that does this
> > particular constant folding comes through dead_libcall_p, a
> > function that no longer exists in 4.4 and later.
> 
> I see.  So you are comfortably sure all other places that call
> simplify-rtx are properly guarded in trunk?  At least I cannot see
> how wrong simplifications are avoided in simplify_binary_operation_1
> there.  Which would be the better place to stick you check so
> that constant folding via dfp.c can still be performed from
> simplify_const_binary_operation?
> 
> Thanks,
> Richard.

No, I'm not comfortable, and while trying to avoid a libcall to
multiply a decimal float value by -1.0dd with negating the value
(which isn't valid for decimal float arithmetic, where trailing
zeroes are significant) I now see that I went too far and turned
off some legitimate (for decimal float) constant folding.

Probably what I really want to do is prevent generating the REG_EQUAL
note that causes the replacement in the 4.3 branch.  The REG_EQUAL is
also generated for trunk and 4.4, but without the call to
dead_libcall_p it's not being replaced in those versions, at least
for this particular test case; it might cause problems elsewhere.

Janis

> > Janis
> >
> >> > Bootstrapped and regression tested on powerpc64-linux, -m32/-m64; OK
> >> > for the 4.3 branch?
> >> >
> >> > 2009-07-15  Janis Johnson  <janis187@us.ibm.com>
> >> >
> >> > gcc/
> >> >        PR c/39902
> >> >        * tree.c (real_zerop, real_onep, real_twop, real_minus_onep):
> >> >        Special-case decimal float constants.
> >> >        * simplify-rtx.c (simplify_binary_operation): Skip for decimal float.
> >> >
> >> > gcc/testsuite/
> >> >        PR c/39902
> >> >        * gcc.dg/dfp/pr39902.c: New test.
> >> >
> >> > Index: gcc/tree.c
> >> > ===================================================================
> >> > --- gcc/tree.c  (revision 148982)
> >> > +++ gcc/tree.c  (working copy)
> >> > @@ -1543,7 +1543,8 @@ tree_floor_log2 (const_tree expr)
> >> >          : floor_log2 (low));
> >> >  }
> >> >
> >> > -/* Return 1 if EXPR is the real constant zero.  */
> >> > +/* Return 1 if EXPR is the real constant zero.  Trailing zeroes matter for
> >> > +   decimal float constants, so don't return 1 for them.  */
> >> >
> >> >  int
> >> >  real_zerop (const_tree expr)
> >> > @@ -1551,13 +1552,16 @@ real_zerop (const_tree expr)
> >> >   STRIP_NOPS (expr);
> >> >
> >> >   return ((TREE_CODE (expr) == REAL_CST
> >> > -          && REAL_VALUES_EQUAL (TREE_REAL_CST (expr), dconst0))
> >> > +          && REAL_VALUES_EQUAL (TREE_REAL_CST (expr), dconst0)
> >> > +          && !(DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (expr)))))
> >> >          || (TREE_CODE (expr) == COMPLEX_CST
> >> >              && real_zerop (TREE_REALPART (expr))
> >> >              && real_zerop (TREE_IMAGPART (expr))));
> >> >  }
> >> >
> >> > -/* Return 1 if EXPR is the real constant one in real or complex form.  */
> >> > +/* Return 1 if EXPR is the real constant one in real or complex form.
> >> > +   Trailing zeroes matter for decimal float constants, so don't return
> >> > +   1 for them.  */
> >> >
> >> >  int
> >> >  real_onep (const_tree expr)
> >> > @@ -1565,13 +1569,15 @@ real_onep (const_tree expr)
> >> >   STRIP_NOPS (expr);
> >> >
> >> >   return ((TREE_CODE (expr) == REAL_CST
> >> > -          && REAL_VALUES_EQUAL (TREE_REAL_CST (expr), dconst1))
> >> > +          && REAL_VALUES_EQUAL (TREE_REAL_CST (expr), dconst1)
> >> > +          && !(DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (expr)))))
> >> >          || (TREE_CODE (expr) == COMPLEX_CST
> >> >              && real_onep (TREE_REALPART (expr))
> >> >              && real_zerop (TREE_IMAGPART (expr))));
> >> >  }
> >> >
> >> > -/* Return 1 if EXPR is the real constant two.  */
> >> > +/* Return 1 if EXPR is the real constant two.  Trailing zeroes matter
> >> > +   for decimal float constants, so don't return 1 for them.  */
> >> >
> >> >  int
> >> >  real_twop (const_tree expr)
> >> > @@ -1579,13 +1585,15 @@ real_twop (const_tree expr)
> >> >   STRIP_NOPS (expr);
> >> >
> >> >   return ((TREE_CODE (expr) == REAL_CST
> >> > -          && REAL_VALUES_EQUAL (TREE_REAL_CST (expr), dconst2))
> >> > +          && REAL_VALUES_EQUAL (TREE_REAL_CST (expr), dconst2)
> >> > +          && !(DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (expr)))))
> >> >          || (TREE_CODE (expr) == COMPLEX_CST
> >> >              && real_twop (TREE_REALPART (expr))
> >> >              && real_zerop (TREE_IMAGPART (expr))));
> >> >  }
> >> >
> >> > -/* Return 1 if EXPR is the real constant minus one.  */
> >> > +/* Return 1 if EXPR is the real constant minus one.  Trailing zeroes
> >> > +   matter for decimal float constants, so don't return 1 for them.  */
> >> >
> >> >  int
> >> >  real_minus_onep (const_tree expr)
> >> > @@ -1593,7 +1601,8 @@ real_minus_onep (const_tree expr)
> >> >   STRIP_NOPS (expr);
> >> >
> >> >   return ((TREE_CODE (expr) == REAL_CST
> >> > -          && REAL_VALUES_EQUAL (TREE_REAL_CST (expr), dconstm1))
> >> > +          && REAL_VALUES_EQUAL (TREE_REAL_CST (expr), dconstm1)
> >> > +          && !(DECIMAL_FLOAT_MODE_P (TYPE_MODE (TREE_TYPE (expr)))))
> >> >          || (TREE_CODE (expr) == COMPLEX_CST
> >> >              && real_minus_onep (TREE_REALPART (expr))
> >> >              && real_zerop (TREE_IMAGPART (expr))));
> >> > Index: gcc/simplify-rtx.c
> >> > ===================================================================
> >> > --- gcc/simplify-rtx.c  (revision 148982)
> >> > +++ gcc/simplify-rtx.c  (working copy)
> >> > @@ -1533,6 +1533,11 @@ simplify_binary_operation (enum rtx_code
> >> >   gcc_assert (GET_RTX_CLASS (code) != RTX_COMPARE);
> >> >   gcc_assert (GET_RTX_CLASS (code) != RTX_COMM_COMPARE);
> >> >
> >> > +  /* Do not simplify decimal floating-point operations, for which trailing
> >> > +     zeroes in constants are significant.  */
> >> > +  if (DECIMAL_FLOAT_MODE_P (mode))
> >> > +     return NULL;
> >> > +
> >> >   /* Make sure the constant is second.  */
> >> >   if (GET_RTX_CLASS (code) == RTX_COMM_ARITH
> >> >       && swap_commutative_operands_p (op0, op1))
> >> > Index: gcc/testsuite/gcc.dg/dfp/pr39902.c
> >> > ===================================================================
> >> > --- gcc/testsuite/gcc.dg/dfp/pr39902.c  (revision 0)
> >> > +++ gcc/testsuite/gcc.dg/dfp/pr39902.c  (revision 0)
> >> > @@ -0,0 +1,239 @@
> >> > +/* { dg-options "--std=gnu99" } */
> >> > +
> >> > +/* Check that optimizations like (x * 1) to x, or (x * -1) to -x,
> >> > +   do not apply to decimal float computations where trailing zeroes
> >> > +   are significant.  */
> >> > +
> >> > +extern void abort (void);
> >> > +int failcnt;
> >> > +
> >> > +#ifdef DBG
> >> > +extern int printf (const char *, ...);
> >> > +#define FAILURE { printf ("failed at line %d\n", __LINE__); failcnt++; }
> >> > +#else
> >> > +#define FAILURE abort ();
> >> > +#endif
> >> > +
> >> > +#define COMPARE32(A,B) \
> >> > +  A.i == B.i
> >> > +
> >> > +#define COMPARE64(A,B) \
> >> > +  A.i[0] == B.i[0] && A.i[1] == B.i[1]
> >> > +
> >> > +#define COMPARE128(A,B) \
> >> > +  A.i[0] == B.i[0] && A.i[1] == B.i[1] && A.i[2] == B.i[2] && A.i[3] == B.i[3]
> >> > +
> >> > +typedef union {
> >> > +  _Decimal32 d;
> >> > +  unsigned int i;
> >> > +} u32;
> >> > +
> >> > +typedef union {
> >> > +  _Decimal64 d;
> >> > +  unsigned int i[2];
> >> > +} u64;
> >> > +
> >> > +typedef union {
> >> > +  _Decimal128 d;
> >> > +  unsigned int i[4];
> >> > +} u128;
> >> > +
> >> > +volatile u32 p32_1;
> >> > +volatile u32 p32_1_0;
> >> > +volatile u32 p32_2_0;
> >> > +volatile u32 m32_1;
> >> > +volatile u32 m32_1_0;
> >> > +volatile u32 m32_2_0;
> >> > +volatile u32 a32;
> >> > +
> >> > +volatile u64 p64_1;
> >> > +volatile u64 p64_1_0;
> >> > +volatile u64 p64_2_0;
> >> > +volatile u64 m64_1;
> >> > +volatile u64 m64_1_0;
> >> > +volatile u64 m64_2_0;
> >> > +volatile u64 a64;
> >> > +
> >> > +volatile u128 p128_1;
> >> > +volatile u128 p128_1_0;
> >> > +volatile u128 p128_2_0;
> >> > +volatile u128 m128_1;
> >> > +volatile u128 m128_1_0;
> >> > +volatile u128 m128_2_0;
> >> > +volatile u128 a128;
> >> > +
> >> > +void
> >> > +init32 (void)
> >> > +{
> >> > +  p32_1.d = 1.DF;
> >> > +  p32_1_0.d = 1.0DF;
> >> > +  p32_2_0.d = 2.0DF;
> >> > +  m32_1.d = -1.DF;
> >> > +  m32_1_0.d = -1.0DF;
> >> > +  m32_2_0.d = -2.0DF;
> >> > +}
> >> > +
> >> > +void
> >> > +init64 (void)
> >> > +{
> >> > +  p64_1.d = 1.DD;
> >> > +  p64_1_0.d = 1.0DD;
> >> > +  p64_2_0.d = 2.0DD;
> >> > +  m64_1.d = -1.DD;
> >> > +  m64_1_0.d = -1.0DD;
> >> > +  m64_2_0.d = -2.0DD;
> >> > +}
> >> > +
> >> > +void
> >> > +init128 (void)
> >> > +{
> >> > +  p128_1.d = 1.DL;
> >> > +  p128_1_0.d = 1.0DL;
> >> > +  p128_2_0.d = 2.0DL;
> >> > +  m128_1.d = -1.DL;
> >> > +  m128_1_0.d = -1.0DL;
> >> > +  m128_2_0.d = -2.0DL;
> >> > +}
> >> > +
> >> > +void
> >> > +doit32 (void)
> >> > +{
> >> > +  /* Multiplying by a value with no trailing zero should not change the
> >> > +     quantum exponent.  */
> >> > +
> >> > +  a32.d = p32_2_0.d * p32_1.d;
> >> > +  if (! (COMPARE32 (a32, p32_2_0)))
> >> > +    FAILURE
> >> > +
> >> > +  a32.d = p32_2_0.d * 1.DF;
> >> > +  if (! (COMPARE32 (a32, p32_2_0)))
> >> > +    FAILURE
> >> > +
> >> > +  a32.d = p32_2_0.d * m32_1.d;
> >> > +  if (! (COMPARE32 (a32, m32_2_0)))
> >> > +    FAILURE
> >> > +
> >> > +  a32.d = p32_2_0.d * -1.DF;
> >> > +  if (! (COMPARE32 (a32, m32_2_0)))
> >> > +    FAILURE
> >> > +
> >> > +  /* Multiplying by a value with a trailing zero should change the
> >> > +     quantum exponent.  */
> >> > +
> >> > +  a32.d = p32_2_0.d * p32_1_0.d;
> >> > +  if (COMPARE32 (a32, p32_2_0))
> >> > +    FAILURE
> >> > +
> >> > +  a32.d = p32_2_0.d * 1.0DF;
> >> > +  if (COMPARE32 (a32, p32_2_0))
> >> > +    FAILURE
> >> > +
> >> > +  a32.d = p32_2_0.d * m32_1_0.d;
> >> > +  if (COMPARE32 (a32, m32_2_0))
> >> > +    FAILURE
> >> > +
> >> > +  a32.d = p32_2_0.d * -1.0DF;
> >> > +  if (COMPARE32 (a32, m32_2_0))
> >> > +    FAILURE
> >> > +}
> >> > +
> >> > +void
> >> > +doit64 (void)
> >> > +{
> >> > +  /* Multiplying by a value with no trailing zero should not change the
> >> > +     quantum exponent.  */
> >> > +
> >> > +  a64.d = p64_2_0.d * p64_1.d;
> >> > +  if (! (COMPARE64 (a64, p64_2_0)))
> >> > +    FAILURE
> >> > +
> >> > +  a64.d = p64_2_0.d * 1.DD;
> >> > +  if (! (COMPARE64 (a64, p64_2_0)))
> >> > +    FAILURE
> >> > +
> >> > +  a64.d = p64_2_0.d * m64_1.d;
> >> > +  if (! (COMPARE64 (a64, m64_2_0)))
> >> > +    FAILURE
> >> > +
> >> > +  a64.d = p64_2_0.d * -1.DD;
> >> > +  if (! (COMPARE64 (a64, m64_2_0)))
> >> > +    FAILURE
> >> > +
> >> > +  /* Multiplying by a value with a trailing zero should change the
> >> > +     quantum exponent.  */
> >> > +
> >> > +  a64.d = p64_2_0.d * p64_1_0.d;
> >> > +  if (COMPARE64 (a64, p64_2_0))
> >> > +    FAILURE
> >> > +
> >> > +  a64.d = p64_2_0.d * 1.0DD;
> >> > +  if (COMPARE64 (a64, p64_2_0))
> >> > +    FAILURE
> >> > +
> >> > +  a64.d = p64_2_0.d * m64_1_0.d;
> >> > +  if (COMPARE64 (a64, m64_2_0))
> >> > +    FAILURE
> >> > +
> >> > +  a64.d = p64_2_0.d * -1.0DD;
> >> > +  if (COMPARE64 (a64, m64_2_0))
> >> > +    FAILURE
> >> > +}
> >> > +
> >> > +void
> >> > +doit128 (void)
> >> > +{
> >> > +  /* Multiplying by a value with no trailing zero should not change the
> >> > +     quantum exponent.  */
> >> > +
> >> > +  a128.d = p128_2_0.d * p128_1_0.d;
> >> > +  if (COMPARE128 (a128, p128_2_0))
> >> > +    FAILURE
> >> > +
> >> > +  a128.d = p128_2_0.d * 1.0DD;
> >> > +  if (COMPARE128 (a128, p128_2_0))
> >> > +    FAILURE
> >> > +
> >> > +  a128.d = p128_2_0.d * m128_1_0.d;
> >> > +  if (COMPARE128 (a128, m128_2_0))
> >> > +    FAILURE
> >> > +
> >> > +  a128.d = p128_2_0.d * -1.0DD;
> >> > +  if (COMPARE128 (a128, m128_2_0))
> >> > +    FAILURE
> >> > +
> >> > +  /* Multiplying by a value with a trailing zero should change the
> >> > +     quantum exponent.  */
> >> > +
> >> > +  a128.d = p128_2_0.d * p128_1.d;
> >> > +  if (! (COMPARE128 (a128, p128_2_0)))
> >> > +    FAILURE
> >> > +
> >> > +  a128.d = p128_2_0.d * 1.DD;
> >> > +  if (! (COMPARE128 (a128, p128_2_0)))
> >> > +    FAILURE
> >> > +
> >> > +  a128.d = p128_2_0.d * m128_1.d;
> >> > +  if (! (COMPARE128 (a128, m128_2_0)))
> >> > +    FAILURE
> >> > +
> >> > +  a128.d = p128_2_0.d * -1.DD;
> >> > +  if (! (COMPARE128 (a128, m128_2_0)))
> >> > +    FAILURE
> >> > +}
> >> > +
> >> > +int
> >> > +main (void)
> >> > +{
> >> > +  init32 ();
> >> > +  init64 ();
> >> > +  init128 ();
> >> > +
> >> > +  doit32 ();
> >> > +  doit64 ();
> >> > +  doit128 ();
> >> > +
> >> > +  if (failcnt != 0)
> >> > +    abort ();
> >> > +
> >> > +  return 0;
> >> > +}
> >> >
> >> >
> >> >
> >
> >