[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 Uroš Bizjak changed: What|Removed |Added Resolution|--- |FIXED Status|ASSIGNED|RESOLVED --- Comment #23 from Uroš Bizjak --- Fixed everywhere.
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #22 from Uroš Bizjak --- https://gcc.gnu.org/g:edb28850520d1137d12a1cc1c0e89c11e6b0c6ef commit r8-10691-gedb28850520d1137d12a1cc1c0e89c11e6b0c6ef Author: Uros Bizjak Date: Wed Dec 23 09:18:12 2020 +0100 i386: Fix __builtin_floor with FE_DOWNWARD rounding direction [PR96793] x86_expand_floorceil expander uses x86_sse_copysign_to_positive, which is unable to change the sign from - to +. When FE_DOWNWARD rounding direction is in effect, the expanded sequence that involves subtraction can trigger x - x = -0.0 special rule. x86_sse_copysign_to_positive fails to change the sign of the intermediate value, assumed to always be positive, back to positive. The patch adds one extra fabs that strips the sign from the intermediate value when flag_rounding_math is in effect. 2020-12-22 Uroš Bizjak gcc/ PR target/96793 * config/i386/i386.c (ix86_expand_floorceil): Remove the sign of the intermediate value for flag_rounding_math. (ix86_expand_floorceildf_32): Ditto. gcc/testsuite/ PR target/96793 * gcc.target/i386/pr96793.c: New test.
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #21 from Uroš Bizjak --- https://gcc.gnu.org/g:c40b640ebcef1aae78eaca56e04d204dda9e4cad commit r9-9126-gc40b640ebcef1aae78eaca56e04d204dda9e4cad Author: Uros Bizjak Date: Wed Dec 23 09:09:29 2020 +0100 i386: Fix __builtin_floor with FE_DOWNWARD rounding direction [PR96793] x86_expand_floorceil expander uses x86_sse_copysign_to_positive, which is unable to change the sign from - to +. When FE_DOWNWARD rounding direction is in effect, the expanded sequence that involves subtraction can trigger x - x = -0.0 special rule. x86_sse_copysign_to_positive fails to change the sign of the intermediate value, assumed to always be positive, back to positive. The patch adds one extra fabs that strips the sign from the intermediate value when flag_rounding_math is in effect. 2020-12-22 Uroš Bizjak gcc/ PR target/96793 * config/i386/i386.c (ix86_expand_floorceil): Remove the sign of the intermediate value for flag_rounding_math. (ix86_expand_floorceildf_32): Ditto. gcc/testsuite/ PR target/96793 * gcc.target/i386/pr96793.c: New test.
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #20 from Uroš Bizjak --- https://gcc.gnu.org/g:0bf0e0b86d3e2f12555479096baaf0ca7a9f7ac6 commit r10-9164-g0bf0e0b86d3e2f12555479096baaf0ca7a9f7ac6 Author: Uros Bizjak Date: Tue Dec 22 21:11:51 2020 +0100 i386: Fix __builtin_floor with FE_DOWNWARD rounding direction [PR96793] x86_expand_floorceil expander uses x86_sse_copysign_to_positive, which is unable to change the sign from - to +. When FE_DOWNWARD rounding direction is in effect, the expanded sequence that involves subtraction can trigger x - x = -0.0 special rule. x86_sse_copysign_to_positive fails to change the sign of the intermediate value, assumed to always be positive, back to positive. The patch adds one extra fabs that strips the sign from the intermediate value when flag_rounding_math is in effect. 2020-12-22 Uroš Bizjak gcc/ PR target/96793 * config/i386/i386-expand.c (ix86_expand_floorceil): Remove the sign of the intermediate value for flag_rounding_math. (ix86_expand_floorceildf_32): Ditto. gcc/testsuite/ PR target/96793 * gcc.target/i386/pr96793.c: New test.
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #19 from Uroš Bizjak --- https://gcc.gnu.org/g:337ed0eb490b14899f4049bc4c8922eb1d8a2e67 commit r11-6303-g337ed0eb490b14899f4049bc4c8922eb1d8a2e67 Author: Uros Bizjak Date: Tue Dec 22 18:13:24 2020 +0100 i386: Fix __builtin_floor with FE_DOWNWARD rounding direction [PR96793] x86_expand_floorceil expander uses x86_sse_copysign_to_positive, which is unable to change the sign from - to +. When FE_DOWNWARD rounding direction is in effect, the expanded sequence that involves subtraction can trigger x - x = -0.0 special rule. x86_sse_copysign_to_positive fails to change the sign of the intermediate value, assumed to always be positive, back to positive. The patch adds one extra fabs that strips the sign from the intermediate value when flag_rounding_math is in effect. 2020-12-22 Uroš Bizjak gcc/ PR target/96793 * config/i386/i386-expand.c (ix86_expand_floorceil): Remove the sign of the intermediate value for flag_rounding_math. (ix86_expand_floorceildf_32): Ditto. gcc/testsuite/ PR target/96793 * gcc.target/i386/pr96793.c: New test.
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 Uroš Bizjak changed: What|Removed |Added Status|NEW |ASSIGNED Assignee|unassigned at gcc dot gnu.org |ubizjak at gmail dot com --- Comment #18 from Uroš Bizjak --- Created attachment 49833 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49833&action=edit Proposed patch Proposed patch that removes the sign from a temporary with FE_DOWNWARD, where x - x = -0.0
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 Uroš Bizjak changed: What|Removed |Added Assignee|ubizjak at gmail dot com |unassigned at gcc dot gnu.org CC||ubizjak at gmail dot com Status|ASSIGNED|NEW --- Comment #17 from Uroš Bizjak --- (In reply to Uroš Bizjak from comment #9) > (In reply to Paweł Bylica from comment #8) > > Did you consider fixing the __builtin_floor() implementation? > > No, because you can use -msse4 to generate ROUNDxx instructions. I will obsolete the patch.
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #16 from Paweł Bylica --- I have checked the glibc implementation of floorf(). Source here: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/ieee754/flt-32/s_floorf.c;h=da6c6dfa8ae86129e74d2e4391fac3a3c2ec;hb=HEAD - It has variant for SSE 4.1 using ROUNDSS 9 - all good here. - Otherwise it either uses __builtin_floorf() (so bug here), - or generic implementation based on bit manipulations (so should be rounding direction independent).
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #15 from Paweł Bylica --- (In reply to Marc Glisse from comment #14) > (In reply to Marc Glisse from comment #13) > > if (HONOR_SIGNED_ZEROS (mode)) > > x2 = copysign (x2, x); > > Hmm, I misread the comment, sorry. We already do that, for both floor and > ceil. But we don't use a true copysign, we use ix86_sse_copysign_to_positive > which won't be able to change the sign from - to +. Just changing it to a > true copysign (one extra and or andn) should be enough then? Yes, having full copysign should do the job.
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #14 from Marc Glisse --- (In reply to Marc Glisse from comment #13) > if (HONOR_SIGNED_ZEROS (mode)) > x2 = copysign (x2, x); Hmm, I misread the comment, sorry. We already do that, for both floor and ceil. But we don't use a true copysign, we use ix86_sse_copysign_to_positive which won't be able to change the sign from - to +. Just changing it to a true copysign (one extra and or andn) should be enough then?
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #13 from Marc Glisse --- x-x does depend on the rounding mode (the transformation in match.pd gets it wrong, by the way). If the sign of 0 is the only issue, maybe we can test flag_rounding_math && flag_signed_zeros or the corresponding HONOR_*(mode)? There are sensible cases where rounding matters but not the sign of 0. As for making the sequence always work... I am not sure there is much better than if(x2==0)x2=0;. We could also compute -1 in type long (the test isless should already guarantee that there is no overflow?), that means an extra conversion from long to double. I see that ix86_expand_floorceildf_32 already ends with if (HONOR_SIGNED_ZEROS (mode)) x2 = copysign (x2, x); so we could also add that to ix86_expand_floorceil.
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #12 from Uroš Bizjak --- (In reply to Richard Biener from comment #10) > (In reply to Uroš Bizjak from comment #7) > > Created attachment 49144 [details] > > Proposed patch > > > > Patch in testing. > > OTOH we _do_ try to compensate for HONOR_SIGNED_ZEROS so I wonder whether > we really need to punt for -frounding-math? I also doubt glibc gets it > correct, does it? The testcase from Comment #11 works OK when it calls floorf from glibc-2.31, but it was ran on SSE4.1 capable target, so roundss was used.
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #11 from Uroš Bizjak --- Created attachment 49146 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49146&action=edit Testcase, suitable for gcc testsuite
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 Richard Biener changed: What|Removed |Added CC||glisse at gcc dot gnu.org --- Comment #10 from Richard Biener --- (In reply to Uroš Bizjak from comment #7) > Created attachment 49144 [details] > Proposed patch > > Patch in testing. OTOH we _do_ try to compensate for HONOR_SIGNED_ZEROS so I wonder whether we really need to punt for -frounding-math? I also doubt glibc gets it correct, does it? I guess /* 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))); tmp = expand_simple_binop (mode, do_floor ? MINUS : PLUS, xa, tmp, NULL_RTX, 0, OPTAB_DIRECT); might be in error for FE_DOWNWARD if 0 - 0 behaves differently depending on rounding mode... or the compensation is off if (HONOR_SIGNED_ZEROS (mode)) ix86_sse_copysign_to_positive (res, res, force_reg (mode, operand1), mask);
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #9 from Uroš Bizjak --- (In reply to Paweł Bylica from comment #8) > Did you consider fixing the __builtin_floor() implementation? No, because you can use -msse4 to generate ROUNDxx instructions.
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #8 from Paweł Bylica --- Did you consider fixing the __builtin_floor() implementation?
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 Uroš Bizjak changed: What|Removed |Added CC|uros at gcc dot gnu.org| Target Milestone|--- |8.5
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 Uroš Bizjak changed: What|Removed |Added Status|NEW |ASSIGNED Assignee|unassigned at gcc dot gnu.org |ubizjak at gmail dot com --- Comment #7 from Uroš Bizjak --- Created attachment 49144 --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49144&action=edit Proposed patch Patch in testing.
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 --- Comment #6 from Uroš Bizjak --- Ehm... 2006-10-29 Richard Guenther * config/i386/i386-protos.h (ix86_expand_floorceil): Declare. (ix86_expand_floorceildf_32): Likewise. * config/i386/i386.c (ix86_expand_sse_compare_mask): New static helper function. (ix86_expand_floorceil): Expander for floor and ceil to SSE math. (ix86_expand_floorceildf_32): Same for DFmode on 32bit archs. * config/i386/i386.md (floordf2): Adjust to enable floor expansion via ix86_expand_floorceil if TARGET_SSE_MATH and -fno-trapping-math is enabled and if not optimizing for size. (floorsf2, ceildf2, ceilsf2): Likewise. * config/i386/sse.md (sse_maskcmpsf3): New insn. (sse2_maskcmpdf3): Likewise.
[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793 Richard Biener changed: What|Removed |Added Last reconfirmed||2020-08-26 CC||uros at gcc dot gnu.org Status|UNCONFIRMED |NEW Ever confirmed|0 |1 Component|c |target Target||x86_64-*-* i?86-*-* --- Comment #5 from Richard Biener --- Confirmed. Looks like the inline-expanded sequence is off with -frounding-math (we also inline expand at -O0 but not -Os which is somewhat surprising).
