llvm dev - Sep 2020 - Invalid transformation in LibCallSimplifier::replacePowWithSqrt?

On Mon, Sep 14, 2020 at 12:45 PM Sanjay Patel <spatel at rotateright.com>
wrote:
> Yes, that looks like a bug. The transform is ok in general for negative
> numbers, but -Inf is a special-case for pow(), right?
> If so, we probably need an extra check of the input with
> "isKnownNeverInfinity()".
>
There is an extra check there already, but it uses "select" instead of
branching. One question is if branching is okay to use instead. Or perhaps
you mean the transform should not be done unless
"isKnownNeverInfinity()"
returns true.

> If there are other errno divergences for edge case values, we may need to
> check other conditions.
>
> On Sat, Sep 12, 2020 at 9:37 PM Hubert Tong via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>> The transformation in LibCallSimplifier::replacePowWithSqrt with
respect
>> to -Inf uses a select instruction, which based on the observed
behaviour,
>> incorporates the side effects of the unchosen branch. This means that
(for
>> pow) a call to sqrt(-Inf) is materialized. Such a call is specified as
>> having a domain error (C17 subclause 7.12.7.5) since the operand is
less
>> than zero. Contrast this with pow(-Inf, 0.5), which is specified by C17
>> subclause F.10.4.4 as having a result of +Inf (indicating an exact
result
>> for the operation and, since IEEE Std 754-2008 subclause 9.1.1 states
that
>> domain errors are to be indicated by a NaN result, a lack of a domain
>> error).
>>
>> It is noted that the above statements were made notwithstanding the
>> ERRORS section of pow() in POSIX.1-2017 XSH Chapter 3, which specifies
a
>> domain error except perhaps by deference to the C standard due to a
>> conflict between the POSIX and the C wording.
>>
>> The transformation in question causes EDOM for  pow(-Inf, 0.5) even on
>> platforms where the system library implementation of pow does not cause
>> this situation to arise.
>>
>> A sample program that (on some platforms, such as Linux on x86-64)
>> completes successfully with optimizations off, and aborts with
LLVM's
>> optimization follows; -fmath-errno does not help, and it is not
expected
>> to, because it is designed to retain setting errno to non-zero (not to
>> prevent spuriously setting errno).
>>
>> #include <errno.h>
>> volatile double inf = -__builtin_inf();
>>
>> double pow(double, double);
>> void abort(void);
>> int main(void) {
>>   errno = 0;
>>   pow(inf, 0.5);
>>   if (errno != 0) abort();
>> }
>>
>> Compiler Explorer link: https://godbolt.org/z/5Wr66M
>>
>> Is the transformation actually valid in some way? If not, I see no
>> instances where the LibCallSimplier generates conditional branches.
>> Retaining the transformation in some way without generating conditional
>> branches would probably involve bailing out if infinities are still in
play.
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200914/6e857a8b/attachment.html>

Yes, I mean just bail out on the transform in
LibCallSimplifier::replacePowWithSqrt() -> getSqrtCall(). If we can't
prove
the call behaves the same with errno, then give up.
I'm not sure where the select / branching happens, but I don't see that
happening in the initial transform (called from -instcombine)

On Mon, Sep 14, 2020 at 12:58 PM Hubert Tong <
hubert.reinterpretcast at gmail.com> wrote:
> On Mon, Sep 14, 2020 at 12:45 PM Sanjay Patel <spatel at
rotateright.com>
> wrote:
>
>> Yes, that looks like a bug. The transform is ok in general for negative
>> numbers, but -Inf is a special-case for pow(), right?
>> If so, we probably need an extra check of the input with
>> "isKnownNeverInfinity()".
>>
> There is an extra check there already, but it uses "select"
instead of
> branching. One question is if branching is okay to use instead. Or perhaps
> you mean the transform should not be done unless
"isKnownNeverInfinity()"
> returns true.
>
>
>> If there are other errno divergences for edge case values, we may need
to
>> check other conditions.
>>
>> On Sat, Sep 12, 2020 at 9:37 PM Hubert Tong via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>>
>>> The transformation in LibCallSimplifier::replacePowWithSqrt with
>>> respect to -Inf uses a select instruction, which based on the
observed
>>> behaviour, incorporates the side effects of the unchosen branch.
This means
>>> that (for pow) a call to sqrt(-Inf) is materialized. Such a call is
>>> specified as having a domain error (C17 subclause 7.12.7.5) since
the
>>> operand is less than zero. Contrast this with pow(-Inf, 0.5), which
is
>>> specified by C17 subclause F.10.4.4 as having a result of +Inf
(indicating
>>> an exact result for the operation and, since IEEE Std 754-2008
subclause
>>> 9.1.1 states that domain errors are to be indicated by a NaN
result, a lack
>>> of a domain error).
>>>
>>> It is noted that the above statements were made notwithstanding the
>>> ERRORS section of pow() in POSIX.1-2017 XSH Chapter 3, which
specifies a
>>> domain error except perhaps by deference to the C standard due to a
>>> conflict between the POSIX and the C wording.
>>>
>>> The transformation in question causes EDOM for  pow(-Inf, 0.5) even
on
>>> platforms where the system library implementation of pow does not
cause
>>> this situation to arise.
>>>
>>> A sample program that (on some platforms, such as Linux on x86-64)
>>> completes successfully with optimizations off, and aborts with
LLVM's
>>> optimization follows; -fmath-errno does not help, and it is not
>>> expected to, because it is designed to retain setting errno to
non-zero
>>> (not to prevent spuriously setting errno).
>>>
>>> #include <errno.h>
>>> volatile double inf = -__builtin_inf();
>>>
>>> double pow(double, double);
>>> void abort(void);
>>> int main(void) {
>>>   errno = 0;
>>>   pow(inf, 0.5);
>>>   if (errno != 0) abort();
>>> }
>>>
>>> Compiler Explorer link: https://godbolt.org/z/5Wr66M
>>>
>>> Is the transformation actually valid in some way? If not, I see no
>>> instances where the LibCallSimplier generates conditional branches.
>>> Retaining the transformation in some way without generating
conditional
>>> branches would probably involve bailing out if infinities are still
in play.
>>>
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org
>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200914/8facfb22/attachment.html>

Sorry - I misread your example and the problem. I see now where
LibCallSimplifier creates the select...but we are immediately erasing that
select with the code from the godbolt example.
Does the real motivating case have no uses of the pow() result value?

On Mon, Sep 14, 2020 at 1:03 PM Sanjay Patel <spatel at rotateright.com>
wrote:
> Yes, I mean just bail out on the transform in
> LibCallSimplifier::replacePowWithSqrt() -> getSqrtCall(). If we
can't prove
> the call behaves the same with errno, then give up.
> I'm not sure where the select / branching happens, but I don't see
that
> happening in the initial transform (called from -instcombine)
>
> On Mon, Sep 14, 2020 at 12:58 PM Hubert Tong <
> hubert.reinterpretcast at gmail.com> wrote:
>
>> On Mon, Sep 14, 2020 at 12:45 PM Sanjay Patel <spatel at
rotateright.com>
>> wrote:
>>
>>> Yes, that looks like a bug. The transform is ok in general for
negative
>>> numbers, but -Inf is a special-case for pow(), right?
>>> If so, we probably need an extra check of the input with
>>> "isKnownNeverInfinity()".
>>>
>> There is an extra check there already, but it uses "select"
instead of
>> branching. One question is if branching is okay to use instead. Or
perhaps
>> you mean the transform should not be done unless
"isKnownNeverInfinity()"
>> returns true.
>>
>>
>>> If there are other errno divergences for edge case values, we may
need
>>> to check other conditions.
>>>
>>> On Sat, Sep 12, 2020 at 9:37 PM Hubert Tong via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> The transformation in LibCallSimplifier::replacePowWithSqrt
with
>>>> respect to -Inf uses a select instruction, which based on the
observed
>>>> behaviour, incorporates the side effects of the unchosen
branch. This means
>>>> that (for pow) a call to sqrt(-Inf) is materialized. Such a
call is
>>>> specified as having a domain error (C17 subclause 7.12.7.5)
since the
>>>> operand is less than zero. Contrast this with pow(-Inf, 0.5),
which is
>>>> specified by C17 subclause F.10.4.4 as having a result of +Inf
(indicating
>>>> an exact result for the operation and, since IEEE Std 754-2008
subclause
>>>> 9.1.1 states that domain errors are to be indicated by a NaN
result, a lack
>>>> of a domain error).
>>>>
>>>> It is noted that the above statements were made notwithstanding
the
>>>> ERRORS section of pow() in POSIX.1-2017 XSH Chapter 3, which
specifies a
>>>> domain error except perhaps by deference to the C standard due
to a
>>>> conflict between the POSIX and the C wording.
>>>>
>>>> The transformation in question causes EDOM for  pow(-Inf, 0.5)
even on
>>>> platforms where the system library implementation of pow does
not
>>>> cause this situation to arise.
>>>>
>>>> A sample program that (on some platforms, such as Linux on
x86-64)
>>>> completes successfully with optimizations off, and aborts with
LLVM's
>>>> optimization follows; -fmath-errno does not help, and it is not
>>>> expected to, because it is designed to retain setting errno to
non-zero
>>>> (not to prevent spuriously setting errno).
>>>>
>>>> #include <errno.h>
>>>> volatile double inf = -__builtin_inf();
>>>>
>>>> double pow(double, double);
>>>> void abort(void);
>>>> int main(void) {
>>>>   errno = 0;
>>>>   pow(inf, 0.5);
>>>>   if (errno != 0) abort();
>>>> }
>>>>
>>>> Compiler Explorer link: https://godbolt.org/z/5Wr66M
>>>>
>>>> Is the transformation actually valid in some way? If not, I see
no
>>>> instances where the LibCallSimplier generates conditional
branches.
>>>> Retaining the transformation in some way without generating
conditional
>>>> branches would probably involve bailing out if infinities are
still in play.
>>>>
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200914/b8550594/attachment.html>