[Bug tree-optimization/103559] New: Can't optimize away < 0 check on sqrt

[Bug tree-optimization/103559] New: Can't optimize away < 0 check on sqrt

[llvm at rifkin dot dev]

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

            Bug ID: 103559
           Summary: Can't optimize away < 0 check on sqrt
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: llvm at rifkin dot dev
  Target Milestone: ---

For a simple invocation of sqrt, gcc inserts a < 0 check to set math errno if
needed. E.g.

float f(float x) {
    return sqrt(x);
}

Is generated as

f(float):
        vxorps  xmm1, xmm1, xmm1
        vucomiss        xmm1, xmm0
        ja      .L10
        vsqrtss xmm0, xmm0, xmm0
        ret
.L10:
        jmp     sqrtf


Unfortunately, this check is still present when the GCC is able to prove that x
is non-negative:

float f(float x) {
    if(x < 0) [[unlikely]] {
        __builtin_unreachable();
    } else {
        return sqrt(x);
    }
}

LLVM suffers from the same problem, even with __builtin_assume().
https://godbolt.org/z/ddcoMj3oz

This is a very common pattern, and I'd imagine the argument for sqrt is often
able to be shown to be positive. This would be a helpful enhancement.

[Bug tree-optimization/103559] Can't optimize away < 0 check on sqrt

[pinskia at gcc dot gnu.org]

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

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I think there is another bug about this. Basically right now there is no jump
threading done for float comparisons nor any kind of VRP for them either.

[Bug tree-optimization/103559] Can't optimize away < 0 check on sqrt

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Oh it is because you are using the wrong test.
Try:
    if (isless (x, 0))
        __builtin_unreachable();

And yes there is a difference.

[Bug tree-optimization/103559] Can't optimize away < 0 check on sqrt

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Also with -fno-trapping-math GCC is able to remove if even for < case.

[Bug tree-optimization/103559] Can't optimize away < 0 check on sqrt

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement
           Keywords|                            |missed-optimization

[Bug tree-optimization/103559] Can't optimize away < 0 check on sqrt

[vanyacpp at gmail dot com]

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

Ivan Sorokin <vanyacpp at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |vanyacpp at gmail dot com

--- Comment #4 from Ivan Sorokin <vanyacpp at gmail dot com> ---
(In reply to Andrew Pinski from comment #1)
> I think there is another bug about this.

Perhaps related to PR91645. The bug report itself is about slightly different
issue, but the comments discusses the same problem.

[Bug tree-optimization/103559] Can't optimize away < 0 check on sqrt

[aldyh at gcc dot gnu.org]

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

Aldy Hernandez <aldyh at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |DUPLICATE
                 CC|                            |aldyh at gcc dot gnu.org

--- Comment #5 from Aldy Hernandez <aldyh at gcc dot gnu.org> ---
(In reply to Jeremy R. from comment #0)
> For a simple invocation of sqrt, gcc inserts a < 0 check to set math errno
> if needed. E.g.
> 
> float f(float x) {
>     return sqrt(x);
> }
> 
> Is generated as
> 
> f(float):
>         vxorps  xmm1, xmm1, xmm1
>         vucomiss        xmm1, xmm0
>         ja      .L10
>         vsqrtss xmm0, xmm0, xmm0
>         ret
> .L10:
>         jmp     sqrtf
> 
> 
> Unfortunately, this check is still present when the GCC is able to prove
> that x is non-negative:
> 
> float f(float x) {
>     if(x < 0) [[unlikely]] {
>         __builtin_unreachable();
>     } else {
>         return sqrt(x);
>     }
> }

x could also be a NAN which I *think* the hardware sqrt won't handle, so a
better test would be:

if (x < 0.0 || __builtin_isnan(x)) [[unlikely]]
  __builtin_unreachable ();

or perhaps:

if (!__builtin_isgreater(x, 0.0))

Either way, this is a subset of PR91645 so I'm closing it as a duplicate.

*** This bug has been marked as a duplicate of bug 91645 ***

[Bug tree-optimization/103559] Can't optimize away < 0 check on sqrt

[llvm at rifkin dot dev]

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

--- Comment #6 from Jeremy R. <llvm at rifkin dot dev> ---
Thanks!

[Bug tree-optimization/103559] Can't optimize away < 0 check on sqrt

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The HW sqrt certainly handles NAN input by returning NAN output, the question
is if it is required to set some errno in that case as well or not.  One would
need to check what different libcs do in that case.

[Bug tree-optimization/103559] Can't optimize away < 0 check on sqrt

[xry111 at gcc dot gnu.org]

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

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

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

--- Comment #8 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #7)
> The HW sqrt certainly handles NAN input by returning NAN output, the
> question is if it is required to set some errno in that case as well or not.
> One would need to check what different libcs do in that case.

Glibc sets EDOM:
https://sourceware.org/git/?p=glibc.git;a=blob;f=math/w_sqrt_template.c

[Bug tree-optimization/103559] Can't optimize away < 0 check on sqrt

[jakub at gcc dot gnu.org]

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

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
isless (NAN, 0.0) should be false, no, so NAN shouldn't errno = EDOM for glibc.

[Bug tree-optimization/103559] Can't optimize away < 0 check on sqrt

[xry111 at gcc dot gnu.org]

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

--- Comment #10 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #9)
> isless (NAN, 0.0) should be false, no, so NAN shouldn't errno = EDOM for
> glibc.

Oh, I misunderstood your question.

[Bug tree-optimization/103559] Can't optimize away < 0 check on sqrt

[jakub at gcc dot gnu.org]

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

--- Comment #11 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Actually, I was just misreading the tree dumps, we use hw insn for x u>= 0.0,
which
is UNGE_EXPR, so it is true if x is NAN or GE_EXPR, or as described in the
tree-call-cdce.cc comment:
        y = sqrt (x);
     ==>
        if (__builtin_isless (x, 0))
          y =  sqrt (x);
        else
          y = IFN_SQRT (x);
So we don't need to inspect anything on the libc sides, we already use the hw
insn even for NANs.
All we need to do is to use the ranger in tree-call-cdce.cc or perhaps later,
and fold the x UNGE_EXPR 0.0 into true whenever frange says so (i.e. that the
range is a subset
of [-0.0, +INF] NAN).