[PATCH] Fix i?86 -mfpmath=sse ceil inline expansion (PR target/89726)
Hi! As the enhanced testcase shows, ix86_expand_floorceildf_32 doesn't emit correct ceil when honoring signed zeros, because it performs copysign, then comparison and then based on the comparison an optional addition of 1. The comment claims that it is essential to use x2 -= -1.0 instead of x2 += 1.0, but in reality we emit x2 += 1.0 anyway and it makes no difference on the sign of result if it is zero, as (unless rounding to negative infinity) both -1.0 - (-1.0) and -1.0 + 1.0 evaluate to +0.0 rather than -0.0 we need in this case. I have no better ideas than to use another copysign, but it should be just one extra instruction, as we've already previously done copysign from the same source and thus hve already computed the x & signmask and so this patch just adds a {,v}por of that previously computed x & signmask into the result again. In this case we aren't really copying it always to positive, but all we want is handle the single problematic case when we compute positive 0 and need negative 0, for all others the oring won't really change the sign. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Or does anybody have some smarter idea? Note, not sure if floor isn't incorrect in the round to negative infinity case (but that would be preexisting issue). 2019-03-15 Jakub Jelinek PR target/89726 * config/i386/i386.c (ix86_expand_floorceildf_32): In ceil compensation use x2 += 1 instead of x2 -= -1 and when honoring signed zeros, do another copysign after the compensation. * gcc.target/i386/fpprec-1.c (x): Add 6 new constants. (expect_round, expect_rint, expect_floor, expect_ceil, expect_trunc): Add expected results for them. --- gcc/config/i386/i386.c.jj 2019-03-14 23:44:27.862560139 +0100 +++ gcc/config/i386/i386.c 2019-03-15 15:07:54.607453430 +0100 @@ -45563,8 +45563,10 @@ ix86_expand_floorceildf_32 (rtx operand0 x2 -= 1; Compensate. Ceil: if (x2 < x) - x2 -= -1; -return x2; + x2 += 1; + if (HONOR_SIGNED_ZEROS (mode)) + x2 = copysign (x2, x); + return x2; */ machine_mode mode = GET_MODE (operand0); rtx xa, TWO52, tmp, one, res, mask; @@ -45590,17 +45592,16 @@ ix86_expand_floorceildf_32 (rtx operand0 /* xa = copysign (xa, operand1) */ ix86_sse_copysign_to_positive (xa, xa, res, mask); - /* generate 1.0 or -1.0 */ - one = force_reg (mode, - const_double_from_real_value (do_floor -? dconst1 : dconstm1, mode)); + /* generate 1.0 */ + one = force_reg (mode, const_double_from_real_value (dconst1, mode)); /* Compensate: xa = xa - (xa > operand1 ? 1 : 0) */ tmp = ix86_expand_sse_compare_mask (UNGT, xa, res, !do_floor); emit_insn (gen_rtx_SET (tmp, gen_rtx_AND (mode, one, tmp))); - /* We always need to subtract here to preserve signed zero. */ - tmp = expand_simple_binop (mode, MINUS, + tmp = expand_simple_binop (mode, do_floor ? MINUS : PLUS, xa, tmp, NULL_RTX, 0, OPTAB_DIRECT); + if (!do_floor && HONOR_SIGNED_ZEROS (mode)) +ix86_sse_copysign_to_positive (tmp, tmp, res, mask); emit_move_insn (res, tmp); emit_label (label); --- gcc/testsuite/gcc.target/i386/fpprec-1.c.jj 2010-05-21 11:46:22.0 +0200 +++ gcc/testsuite/gcc.target/i386/fpprec-1.c2019-03-15 15:01:51.430345113 +0100 @@ -11,6 +11,9 @@ double x[] = { __builtin_nan(""), __buil 0x1.1p-1, 0x1.fp-2, 0x1.1p+0, 0x1.fp-1, 0x1.80001p+0, 0x1.7p+0, + -0x1.1p-1, -0x1.fp-2, + -0x1.1p+0, -0x1.fp-1, + -0x1.80001p+0, -0x1.7p+0, -0.0, 0.0, -0.5, 0.5, -1.0, 1.0, -1.5, 1.5, -2.0, 2.0, -2.5, 2.5 }; #define NUM (sizeof(x)/sizeof(double)) @@ -19,6 +22,7 @@ double expect_round[] = { __builtin_nan( -0x1.fp+1023, 0x1.fp+1023, -0.0, 0.0, 1.0, 0.0, 1.0, 1.0, 2.0, 1.0, + -1.0, -0.0, -1.0, -1.0, -2.0, -1.0, -0.0, 0.0, -1.0, 1.0, -1.0, 1.0, -2.0, 2.0, -2.0, 2.0, -3.0, 3.0 }; @@ -26,6 +30,7 @@ double expect_rint[] = { __builtin_nan(" -0x1.fp+1023, 0x1.fp+1023, -0.0, 0.0, 1.0, 0.0, 1.0, 1.0, 2.0, 1.0, +-1.0, -0.0, -1.0, -1.0, -2.0, -1.0, -0.0, 0.0, -0.0, 0.0, -1.0, 1.0, -2.0, 2.0, -2.0, 2.0, -2.0, 2.0 }; @@ -33,6 +38,7 @@ double expect_floor[] = { __builtin_nan( -0x1.fp+1023, 0x1.fp+1023, -1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 1.0, 1.0, +-1.0, -1.0, -2.0, -1.0, -2.0, -2.0, -0.0, 0.0, -1.0, 0.0, -1.0, 1.0, -2.0, 1.0, -2.0, 2.0, -3.0, 2.0 }; @@ -40,6 +46,7 @@ double expect_ceil[] = { __builtin_nan(" -0x1.fp+1023, 0x1.fp+1023,
Re: [PATCH] Fix i?86 -mfpmath=sse ceil inline expansion (PR target/89726)
On Fri, Mar 15, 2019 at 9:46 PM Jakub Jelinek wrote: > > Hi! > > As the enhanced testcase shows, ix86_expand_floorceildf_32 doesn't emit > correct ceil when honoring signed zeros, because it performs copysign, > then comparison and then based on the comparison an optional addition of 1. > The comment claims that it is essential to use x2 -= -1.0 instead of > x2 += 1.0, but in reality we emit x2 += 1.0 anyway and it makes no > difference on the sign of result if it is zero, as (unless rounding to > negative infinity) both -1.0 - (-1.0) and -1.0 + 1.0 evaluate to +0.0 > rather than -0.0 we need in this case. > > I have no better ideas than to use another copysign, but it should be just > one extra instruction, as we've already previously done copysign from the > same source and thus hve already computed the x & signmask and so this patch > just adds a {,v}por of that previously computed x & signmask into the result > again. In this case we aren't really copying it always to positive, but > all we want is handle the single problematic case when we compute positive 0 > and need negative 0, for all others the oring won't really change the sign. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > Or does anybody have some smarter idea? > > Note, not sure if floor isn't incorrect in the round to negative infinity > case (but that would be preexisting issue). > > 2019-03-15 Jakub Jelinek > > PR target/89726 > * config/i386/i386.c (ix86_expand_floorceildf_32): In ceil > compensation use x2 += 1 instead of x2 -= -1 and when honoring > signed zeros, do another copysign after the compensation. > > * gcc.target/i386/fpprec-1.c (x): Add 6 new constants. > (expect_round, expect_rint, expect_floor, expect_ceil, expect_trunc): > Add expected results for them. LGTM. Thanks, Uros. > --- gcc/config/i386/i386.c.jj 2019-03-14 23:44:27.862560139 +0100 > +++ gcc/config/i386/i386.c 2019-03-15 15:07:54.607453430 +0100 > @@ -45563,8 +45563,10 @@ ix86_expand_floorceildf_32 (rtx operand0 >x2 -= 1; > Compensate. Ceil: > if (x2 < x) > - x2 -= -1; > -return x2; > + x2 += 1; > + if (HONOR_SIGNED_ZEROS (mode)) > + x2 = copysign (x2, x); > + return x2; > */ >machine_mode mode = GET_MODE (operand0); >rtx xa, TWO52, tmp, one, res, mask; > @@ -45590,17 +45592,16 @@ ix86_expand_floorceildf_32 (rtx operand0 >/* xa = copysign (xa, operand1) */ >ix86_sse_copysign_to_positive (xa, xa, res, mask); > > - /* generate 1.0 or -1.0 */ > - one = force_reg (mode, > - const_double_from_real_value (do_floor > -? dconst1 : dconstm1, mode)); > + /* generate 1.0 */ > + one = force_reg (mode, const_double_from_real_value (dconst1, mode)); > >/* Compensate: xa = xa - (xa > operand1 ? 1 : 0) */ >tmp = ix86_expand_sse_compare_mask (UNGT, xa, res, !do_floor); >emit_insn (gen_rtx_SET (tmp, gen_rtx_AND (mode, one, tmp))); > - /* We always need to subtract here to preserve signed zero. */ > - tmp = expand_simple_binop (mode, MINUS, > + tmp = expand_simple_binop (mode, do_floor ? MINUS : PLUS, > xa, tmp, NULL_RTX, 0, OPTAB_DIRECT); > + if (!do_floor && HONOR_SIGNED_ZEROS (mode)) > +ix86_sse_copysign_to_positive (tmp, tmp, res, mask); >emit_move_insn (res, tmp); > >emit_label (label); > --- gcc/testsuite/gcc.target/i386/fpprec-1.c.jj 2010-05-21 11:46:22.0 > +0200 > +++ gcc/testsuite/gcc.target/i386/fpprec-1.c2019-03-15 15:01:51.430345113 > +0100 > @@ -11,6 +11,9 @@ double x[] = { __builtin_nan(""), __buil > 0x1.1p-1, 0x1.fp-2, > 0x1.1p+0, 0x1.fp-1, > 0x1.80001p+0, 0x1.7p+0, > + -0x1.1p-1, -0x1.fp-2, > + -0x1.1p+0, -0x1.fp-1, > + -0x1.80001p+0, -0x1.7p+0, > -0.0, 0.0, -0.5, 0.5, -1.0, 1.0, -1.5, 1.5, -2.0, 2.0, > -2.5, 2.5 }; > #define NUM (sizeof(x)/sizeof(double)) > @@ -19,6 +22,7 @@ double expect_round[] = { __builtin_nan( > -0x1.fp+1023, 0x1.fp+1023, > -0.0, 0.0, > 1.0, 0.0, 1.0, 1.0, 2.0, 1.0, > + -1.0, -0.0, -1.0, -1.0, -2.0, -1.0, > -0.0, 0.0, -1.0, 1.0, -1.0, 1.0, -2.0, 2.0, -2.0, 2.0, > -3.0, 3.0 }; > > @@ -26,6 +30,7 @@ double expect_rint[] = { __builtin_nan(" > -0x1.fp+1023, 0x1.fp+1023, > -0.0, 0.0, > 1.0, 0.0, 1.0, 1.0, 2.0, 1.0, > +-1.0, -0.0, -1.0, -1.0, -2.0, -1.0, > -0.0, 0.0, -0.0, 0.0, -1.0, 1.0, -2.0, 2.0, -2.0, 2.0, > -2.0, 2.0 }; > > @@ -33,6 +38,7 @@ double expect_floor[] = { __builtin_nan( > -0x1.fp+1023, 0x1.fp+1023, > -1.0, 0.0, > 0.0, 0.