[Bug c/96793] New: __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD

[Bug c/96793] New: __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD

[chfast at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

            Bug ID: 96793
           Summary: __builtin_floor produces wrong result when rounding
                    direction is FE_DOWNWARD
           Product: gcc
           Version: 10.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: chfast at gmail dot com
  Target Milestone: ---

Created attachment 49125
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49125&action=edit
Preprocessed test code

If the rounding direction is set to FE_DOWNWARD by fesetround(),
the __builtin_floor() result is -0 where it should be +0. Eg.
__builtin_floor(0.25) == -0.

The outputs are done with GCC 10.2.0 (x86_64-linux-gnu) with -O2
-frounding-math -lm. Full gcc -v output in cli.log file. This is also
reproducible in GCC 9 and trunk.


The test code to reproduce the bug checks if the result's sign is 0 as expected
(attached as builtin_floor_test.c):

enum { FE_DOWNWARD = 0x400 };

extern int fesetround(int rounding_direction);

__attribute__((noinline))
float builtin_floorf(float value)
{
    return __builtin_floorf(value);
}

int main()
{
    fesetround(FE_DOWNWARD);
    float result = builtin_floorf(0.25f);
    return __builtin_signbitf(result) != 0;
}


The __builtin_floor() generates the following assembly (part of
builtin_floor_test.s):

        movss   .LC1(%rip), %xmm2
        movss   .LC0(%rip), %xmm4
        movaps  %xmm0, %xmm3
        movaps  %xmm0, %xmm1
        andps   %xmm2, %xmm3
        ucomiss %xmm3, %xmm4
        jbe     .L2
        cvttss2sil      %xmm0, %eax
        pxor    %xmm3, %xmm3
        andnps  %xmm1, %xmm2
        cvtsi2ssl       %eax, %xmm3
        movaps  %xmm3, %xmm4
        cmpnless        %xmm0, %xmm4
        movss   .LC2(%rip), %xmm0
        andps   %xmm0, %xmm4
        subss   %xmm4, %xmm3
        movaps  %xmm3, %xmm0
        orps    %xmm2, %xmm0
.L2:
        ret

[Bug c/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD

[chfast at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #1 from Paweł Bylica <chfast at gmail dot com> ---
Created attachment 49126
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49126&action=edit
Test source code

[Bug c/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD

[chfast at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #2 from Paweł Bylica <chfast at gmail dot com> ---
Created attachment 49127
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49127&action=edit
Assembly output

[Bug c/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD

[chfast at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #3 from Paweł Bylica <chfast at gmail dot com> ---
Created attachment 49128
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49128&action=edit
Compiler invocation log

[Bug c/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD

[chfast at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #4 from Paweł Bylica <chfast at gmail dot com> ---
I missed some information:

This affects both double and float variants: __builtin_floor() and
__builtin_floorf().

This affects also usage of floor() C standard library function as the function
call usually replaced with __builtin_floor() in optimized builds.

This affects also libstdc++ where std::floor() is implemented with
__builtin_floor().

[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

Richard Biener <rguenth at gcc dot gnu.org> 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 <rguenth at gcc dot gnu.org> ---
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).

[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #6 from Uroš Bizjak <ubizjak at gmail dot com> ---
Ehm...

2006-10-29  Richard Guenther  <rguenther@suse.de>

        * 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

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

Uroš Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |ubizjak at gmail dot com

--- Comment #7 from Uroš Bizjak <ubizjak at gmail dot com> ---
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

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

Uroš Bizjak <ubizjak at gmail dot com> 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

[chfast at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #8 from Paweł Bylica <chfast at gmail dot com> ---
Did you consider fixing the __builtin_floor() implementation?

[Bug target/96793] __builtin_floor produces wrong result when rounding direction is FE_DOWNWARD

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #9 from Uroš Bizjak <ubizjak at gmail dot com> ---
(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

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |glisse at gcc dot gnu.org

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
(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

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #11 from Uroš Bizjak <ubizjak at gmail dot com> ---
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

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #12 from Uroš Bizjak <ubizjak at gmail dot com> ---
(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

[glisse at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #13 from Marc Glisse <glisse at gcc dot gnu.org> ---
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

[glisse at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #14 from Marc Glisse <glisse at gcc dot gnu.org> ---
(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

[chfast at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #15 from Paweł Bylica <chfast at gmail dot com> ---
(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

[chfast at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #16 from Paweł Bylica <chfast at gmail dot com> ---
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=da6c6dfa8ae86129e74dffff2e4391fac3a3c2ec;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

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

Uroš Bizjak <ubizjak at gmail dot com> 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 <ubizjak at gmail dot com> ---
(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

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

Uroš Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |ubizjak at gmail dot com

--- Comment #18 from Uroš Bizjak <ubizjak at gmail dot com> ---
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

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #19 from Uroš Bizjak <ubizjak at gmail dot com> ---


https://gcc.gnu.org/g:337ed0eb490b14899f4049bc4c8922eb1d8a2e67

commit r11-6303-g337ed0eb490b14899f4049bc4c8922eb1d8a2e67
Author: Uros Bizjak <ubizjak@gmail.com>
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  <ubizjak@gmail.com>

    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

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #20 from Uroš Bizjak <ubizjak at gmail dot com> ---
https://gcc.gnu.org/g:0bf0e0b86d3e2f12555479096baaf0ca7a9f7ac6

commit r10-9164-g0bf0e0b86d3e2f12555479096baaf0ca7a9f7ac6
Author: Uros Bizjak <ubizjak@gmail.com>
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  <ubizjak@gmail.com>

    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

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #21 from Uroš Bizjak <ubizjak at gmail dot com> ---


https://gcc.gnu.org/g:c40b640ebcef1aae78eaca56e04d204dda9e4cad

commit r9-9126-gc40b640ebcef1aae78eaca56e04d204dda9e4cad
Author: Uros Bizjak <ubizjak@gmail.com>
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  <ubizjak@gmail.com>

    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

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

--- Comment #22 from Uroš Bizjak <ubizjak at gmail dot com> ---


https://gcc.gnu.org/g:edb28850520d1137d12a1cc1c0e89c11e6b0c6ef

commit r8-10691-gedb28850520d1137d12a1cc1c0e89c11e6b0c6ef
Author: Uros Bizjak <ubizjak@gmail.com>
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  <ubizjak@gmail.com>

    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

[ubizjak at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96793

Uroš Bizjak <ubizjak at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #23 from Uroš Bizjak <ubizjak at gmail dot com> ---
Fixed everywhere.