[Bug middle-end/107879] New: [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[Bug middle-end/107879] New: [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[slyfox at gcc dot gnu.org]

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

            Bug ID: 107879
           Summary: [13 Regression] ffmpeg-4 test suite fails on FPU
                    arithmetics
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: slyfox at gcc dot gnu.org
  Target Milestone: ---

Test failure is initially observed on ffmpeg-4.4.2 test suite where a bunch of
tests started failing comparison against golden values. Extracted minimized
example:

// $ cat a.c
/*
Extracted from ffmpeg-4 test failure initially.

How to reproduce:

    $ gcc-13/bin/gcc -O0 -lm a.c -o a -mfpmath=sse -fexcess-precision=standard
-Wall -Wextra && ./a
    ...
    OK

    $ gcc-13/bin/gcc -O1 -lm a.c -o a -mfpmath=sse -fexcess-precision=standard
-Wall -Wextra && ./a
    ...
    BUG
*/
#include <stdio.h>

__attribute__((noinline, noipa))
static double build_filter(double * y) {
    volatile int ph = 0;
    volatile double vf = 1.0;
    double factor = vf;

    double x = - (double)ph * factor; /* should be -0.0 */
    fprintf(stderr, "z = %f\n", x);   /* prints -0.0, ok */

    /* This 'if / else' code should not affect calculation of 'ffm', but
       removing it does change things. */
    if (x == 0) *y = 1.0;    /* should be 1.0 or .. */
    else *y = 1.0 / x;       /* -inf? */
    fprintf(stderr, "*y = %f\n", *y); /* printf 1.0, ok */

    double w = 2.0 * x / factor; /* should be -0.0 */
    fprintf(stderr, "w = %f\n", w); /* prints -0.0, ok */

    double omww = 1 - w;                  /* should be 1.0 */
    fprintf(stderr, "omww = %f\n", omww); /* printf 1.0, ok */
    double ffm = (omww > 0.0) ? omww : 0.0; /* should be 1.0 */
    fprintf(stderr, "ffm = %f\n", ffm); /* printf 0.0 or 1.0, BUG */

    return ffm;
}

int main()
{
    double y = 42.0;
    double filter = build_filter(&y);

    fprintf(stderr, "f = %.20f; y = %.20f\n", filter, y);
    /* Should be 1.0, sometimes returns 0.0. */
    fprintf(stderr, "%s\n", (filter > 0.5) ? "OK" : "BUG");
}

How to reproduce:

# -O0, good:
$ gcc-HEAD/bin/gcc -O0 -lm a.c -o a -mfpmath=sse -fexcess-precision=standard
-Wall -Wextra && ./a
z = -0.000000
*y = 1.000000
w = -0.000000
omww = 1.000000
ffm = 1.000000
f = 1.00000000000000000000; y = 1.00000000000000000000
OK

# -O1, bad:
$ gcc-HEAD/bin/gcc -O1 -lm a.c -o a -mfpmath=sse -fexcess-precision=standard
-Wall -Wextra && ./a
z = -0.000000
*y = 1.000000
w = -0.000000
omww = 1.000000
ffm = 0.000000
f = 0.00000000000000000000; y = 1.00000000000000000000
BUG

I think the code does not have anything controversial. Probably a code
generation bug?

$ LANG=C gcc-HEAD/bin/gcc -v
Using built-in specs.
COLLECT_GCC=/<<NIX>>/gcc-13.0.0/bin/gcc
COLLECT_LTO_WRAPPER=/<<NIX>>/gcc-13.0.0/libexec/gcc/x86_64-unknown-linux-gnu/13.0.0/lto-wrapper
Target: x86_64-unknown-linux-gnu
Configured with:
Thread model: posix
Supported LTO compression algorithms: zlib
gcc version 13.0.0 20221126 (experimental) (GCC)

[Bug middle-end/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[amonakov at gcc dot gnu.org]

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

Alexander Monakov <amonakov at gcc dot gnu.org> changed:

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

--- Comment #1 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
Yes, thanks for the report. OK with -fno-tree-dominator-opts.

The dom2/dom3 passes duplicate most of the computations in build_filter for the
'x == 0' branch, but the phi node in the resulting basic block 5 incorrectly
receives 0.0 (from bb 6) as the value of 'ffm' computed on the duplicated path
(should be 1.0).

[Bug tree-optimization/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
   Target Milestone|---                         |13.0
                 CC|                            |aldyh at gcc dot gnu.org
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2022-11-26
     Ever confirmed|0                           |1
          Component|middle-end                  |tree-optimization

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
.

[Bug tree-optimization/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[slyfox at gcc dot gnu.org]

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

Sergei Trofimovich <slyfox at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at redhat dot com

--- Comment #3 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
If I did the bisection correctly `git bisect` points to
r13-3926-gd4c2f1d376da6f

commit d4c2f1d376da6fc3f3c30a9d3160e43c95399343
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Nov 12 09:39:00 2022 +0100

    range-op: Implement op[12]_range operators for {PLUS,MINUS,MULT,RDIV}_EXPR

[Bug tree-optimization/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

[Bug tree-optimization/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I will have a look.

[Bug tree-optimization/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[jakub at gcc dot gnu.org]

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

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
It is the foperator_mult::op{1,2}_range and can be seen even on:
double
foo (double x, double y)
{
  double z = x * y;
  if (z == 0.0)
    return x;
  return 42.0;
}
testcase.
2->3  (T) x_2(D) :      [frange] double [-0.0 (-0x0.0p+0), 0.0 (0x0.0p+0)]
2->3  (T) y_3(D) :      [frange] double [-0.0 (-0x0.0p+0), 0.0 (0x0.0p+0)]
2->3  (T) z_4 :         [frange] double [-0.0 (-0x0.0p+0), 0.0 (0x0.0p+0)]
On the true edge of z == 0.0, we have [-0., 0.] range for z_4, that is correct,
but getting from lhs [-0., 0.] and op2 range VARYING to [-0., 0.] is wrong.
We get that because we compute the range in that case as [-0., 0.] / VARYING,
and for division it is actually correct, [-0., 0.] divided by anything non-NAN
non-zero is still [-0., 0.], and [-0., 0.] divided by [-0., 0.] is NAN.
Plus we have the (IMHO still correct) implication that if a result is not NAN,
then neither operand could be NAN.

So I'm afraid back to the drawing board on when we can use division for
multiplication reverse ranges and when we can use multiplication for division
reverse ranges (and whether it is ok to use minus/plus for plus/minus reverse
operations).

[Bug tree-optimization/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 53978
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=53978&action=edit
gcc13-pr107879.patch

Untested fix.

[Bug tree-optimization/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[slyfox at gcc dot gnu.org]

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

--- Comment #7 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #6)
> Created attachment 53978 [details]
> gcc13-pr107879.patch
> 
> Untested fix.

The patch fixed real ffmpeg-4 tests for me (before the change there were about
10 failures in resampler). Thank you!

[Bug tree-optimization/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:4500baaccb6e4d696e223c338bbdf7705c3646dd

commit r13-4492-g4500baaccb6e4d696e223c338bbdf7705c3646dd
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Dec 5 11:17:42 2022 +0100

    range-op-float: Fix up multiplication and division reverse operation
[PR107879]

    While for the normal cases it seems to be correct to implement
    reverse multiplication (op1_range/op2_range) through division
    with float_binary_op_range_finish, reverse division (op1_range)
    through multiplication with float_binary_op_range_finish or
    (op2_range) through division with float_binary_op_range_finish,
    as e.g. following testcase shows for the corner cases it is
    incorrect.
    Say on the testcase we are doing reverse multiplication, we
    have [-0., 0.] range (no NAN) on lhs and VARYING on op1 (or op2).
    We implement that through division, because x from
    lhs = x * op2
    is
    x = lhs / op2
    For the division, [-0., 0.] / VARYING is computed (IMHO correctly)
    as [-0., 0.] +-NAN, because 0 / anything but 0 or NAN is still
    0 and 0 / 0 is NAN and ditto 0 / NAN.  And then we just
    float_binary_op_range_finish, which figures out that because lhs
    can't be NAN, neither operand can be NAN.  So, the end range is
    [-0., 0.].  But that is not correct for the reverse multiplication.
    When the result is 0, if op2 can be zero, then x can be anything
    (VARYING), to be precise anything but INF (unless result can be NAN),
    because anything * 0 is 0 (or NAN for INF).  While if op2 must be
    non-zero, then x must be 0.  Of course the sign logic
    (signbit(x) = signbit(lhs) ^ signbit(op2)) still holds, so it actually
    isn't full VARYING if both lhs and op2 have known sign bits.
    And going through other corner cases one by one shows other differences
    between what we compute for the corresponding forward operation and
    what we should compute for the reverse operations.
    The following patch is slightly conservative and includes INF
    (in case of result including 0 and not NAN) in the ranges or
    0 in the ranges (in case of result including INF and not NAN).
    The latter is what happens anyway because we flush denormals to 0,
    and the former just not to deal with all the corner cases.
    So, the end test is that for reverse multiplication and division
    op2_range the cases we need to adjust to VARYING or VARYING positive
    or VARYING negative are if lhs and op? ranges both contain 0,
    or both contain some infinity, while for division op1_range the
    corner case is if lhs range contains 0 and op2 range contains INF or vice
    versa.  Otherwise I believe ranges from the corresponding operation
    are ok, or could be slightly more conservative (e.g. for
    reverse multiplication, if op? range is singleton INF and lhs
    range doesn't include any INF, then x's range should be UNDEFINED or
    known NAN (depending on if lhs can be NAN), while the division computes
    [-0., 0.] +-NAN; or similarly if op? range is only 0 and lhs range
    doesn't include 0, division would compute +INF +-NAN, or -INF +-NAN,
    or (for lack of multipart franges -INF +INF +-NAN just VARYING +-NAN),
    while again it is UNDEFINED or known NAN.

    Oh, and I found by code inspection wrong condition for the division's
    known NAN result, due to thinko it would trigger not just when
    both operands are known to be 0 or both are known to be INF, but
    when either both are known to be 0, or at least one is known to be INF.

    2022-12-05  Jakub Jelinek  <jakub@redhat.com>

            PR tree-optimization/107879
            * range-op-float.cc (foperator_mult::op1_range): If both
            lhs and op2 ranges contain zero or both ranges contain
            some infinity, set r range to zero_to_inf_range depending on
            signbit_known_p.
            (foperator_div::op2_range): Similarly for lhs and op1 ranges.
            (foperator_div::op1_range): If lhs range contains zero and op2
            range contains some infinity or vice versa, set r range to
            zero_to_inf_range depending on signbit_known_p.
            (foperator_div::rv_fold): Fix up condition for returning known NAN.

[Bug tree-optimization/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.

[Bug tree-optimization/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[amonakov at gcc dot gnu.org]

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

--- Comment #10 from Alexander Monakov <amonakov at gcc dot gnu.org> ---
If anyone is confused like I was, the commit actually includes a testcase, but
the addition is not mentioned in the Changelog. I was sure the server-side
receive hook was supposed to reject such incomplete Changelog, though?

[Bug tree-optimization/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[marxin at gcc dot gnu.org]

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

--- Comment #11 from Martin Liška <marxin at gcc dot gnu.org> ---
> I was sure the
> server-side receive hook was supposed to reject such incomplete Changelog,
> though?

New files in test-suite are not mandatory by the hook and corresponding
ChangeLog entries are generated automatically.

[Bug tree-optimization/107879] [13 Regression] ffmpeg-4 test suite fails on FPU arithmetics

[slyfox at gcc dot gnu.org]

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

--- Comment #12 from Sergei Trofimovich <slyfox at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #5)
> It is the foperator_mult::op{1,2}_range and can be seen even on:
> double
> foo (double x, double y)
> {
>   double z = x * y;
>   if (z == 0.0)
>     return x;
>   return 42.0;
> }
> testcase.
> 2->3  (T) x_2(D) :      [frange] double [-0.0 (-0x0.0p+0), 0.0 (0x0.0p+0)]
> 2->3  (T) y_3(D) :      [frange] double [-0.0 (-0x0.0p+0), 0.0 (0x0.0p+0)]
> 2->3  (T) z_4 :         [frange] double [-0.0 (-0x0.0p+0), 0.0 (0x0.0p+0)]
> On the true edge of z == 0.0, we have [-0., 0.] range for z_4, that is

For my curiosity fow did you get such a detailed [frange] output for the
expressions? I tried -fdump-tree-all-all and best I get is a one-liner around
PHI nodes:

  # RANGE [frange] double [0.0 (0x0.0p+0), +Inf]
  # iftmp.5_6 = PHI <omww_17(3), 0.0(4), 0.0(6)>
  # .MEM_23 = PHI <.MEM_19(3), .MEM_19(4), .MEM_15(6)>
  # VUSE <.MEM_23>
  return iftmp.5_6;

Is there a magic gcc flag to dump more range details? Or you had to patch a bit
of gcc code?