[llvm-dev] Invalid transformation in LibCallSimplifier::replacePowWithSqrt?

[Hubert Tong via llvm-dev]

On Mon, Sep 14, 2020 at 3:44 PM Sanjay Patel <spatel at rotateright.com> wrote:

> Understood, but I think an example that uses the result demonstrates the
> problem more accurately:
> https://godbolt.org/z/n4fGe4
>
> And I'm not sure how to solve that other than my earlier suggestion to
> just give up if the pow() call accesses memory (errno).
>
Thanks for pointing this out. I didn't quite catch this aspect from your
previous note (which covered checking isKnownNeverInfinity()). When the
pow() call is known to not access memory, the code uses the llvm.sqrt.*
intrinsics (and so the transformation is okay).


> On Mon, Sep 14, 2020 at 3:15 PM Hubert Tong <
> hubert.reinterpretcast at gmail.com> wrote:
>
>> On Mon, Sep 14, 2020 at 3:00 PM Sanjay Patel <spatel at rotateright.com>
>> wrote:
>>
>>> 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?
>>>
>> Using the result value (assign to global volatile) doesn't cause the sqrt
>> call to be avoided either.
>>
>>
>>>
>>> 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/bdab7a04/attachment.html>