fast-math: fix a bug related to square roots of negatives

Mon Nov 7 10:57:04 PST 2016

Hi,

On Mon, Nov 7, 2016 at 3:09 AM, Octav Zlatior via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

>
> Hi,
> Those were all very compelling arguments indeed, but I still think it's
> something we could fix. While other compilers such as gcc are not
> necessarily an authority on the issue, I think we should avoid
> inconsistencies.
>
> For example, the fact that sqrt(-2) is undefined, but log(-2) seems to
> nicely turn into NaN eventually. That feels rather broken to me.
>
>
I agree. This may be a missed optimization - we may want to fold log(-2) to
undef as well.

> Or the fact that when building with -ffast-math or -Ofast, the behaviour
> of the code is changes completely (like in the xalanc benchmark, where it
> jumps into an endless loop because of it and eventually segfaults due to a
> stack overflow).
> The point being that this will introduce a lot of weird side effects in
> peoples binaries, and I'd rather fix it now in one place in the compiler
> than have everybody fix it in their (or in 3rd party) code, which most
> likely is not going to happen and people will happily blame it on the
> compiler or the way optimisation passes are implemented.
>
>
If you pass -ffinite-math-only (which -ffast-math implies) and your code
relies on correctly handling NaNs, then, yes, your code will break, in
potentially "weird" ways. Fixing this in your (or in 3rd party) code sounds
like the right thing to me. This is, I think, different from the standard
"how badly should we break undefined behavior" arguments, since we're
talking about the user explicitly asking the compiler to make specific
assumptions.

It's true that GCC handles this differently. I'm not a GCC developer, and
can't speak for them, but I wouldn't be surprised if the only reason for
that is to avoid breaking SPEC. I think our approach to this has been
consistently different.

I can try to change it into a more consistent fix, by either:
> * removing the transform completely, as suggested by Eli, and leave the
> rest to the optimiser
> * changing the undef to NaN (not very sure what the implications of that
> would be of even if it is possible, but it seems to be a sensible solution)
> * trying to extend the current fix/workaround to cover the cases when the
> argument is not a constant, but this seems to be a little more complicated
>
> I'll pick one later today, but I am open to suggestions :)
>
> @Eli: Good catch! I've noticed this as well, on the other hand from my
> observations this would only happen if the value of the argument is known
> (for example if it is specifically assigned in the same scope). It seems to
> work fine if the argument is calculated somewhere else and passed on, which
> is probably the typical usecase.
> I could remove that transform completely, that was actually my first
> workaround to the issue.
> Thanks!
>
> On 07 Nov 2016, at 08:51, Michael Kuperstein via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> This has been raised several times (xalanc failing with -ffast-math isn't
> new), and I believe the last time around we've explicitly decided to keep
> this as undef.
>
> A few relevant threads:
>
> http://lists.llvm.org/pipermail/llvm-commits/Week-
> of-Mon-20140609/220450.html
> http://lists.llvm.org/pipermail/llvm-dev/2014-September/077137.html
> http://llvm.org/bugs/show_bug.cgi?id=21048
>
> Thanks,
>   Michael
>
> On 6 November 2016 at 23:31, Vedant Kumar via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> The optimizer doesn't handle this right now, although it probably could
>> since
>> -instsimplify runs pretty late in the pipeline.
>>
>> Do you think extending the LibCallSimplifier is the right approach?
>>
>> best,
>> vedant
>>
>> P.S: w.r.t the problem that motivated this patch, I think a better
>> solution
>> would be to go through the test programs and change their uses of sqrt().
>>
>>
>> > On Nov 6, 2016, at 9:31 AM, Friedman, Eli via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>> >
>> > On 11/4/2016 11:15 AM, Octav Zlatior via llvm-commits wrote:
>> >>
>> >> Hi,
>> >>
>> >> I have noticed the following behaviour while running the SPEC
>> benchmark suite (built with llvm, of course). One of the tests was failing
>> (Xalanc) with a segfault (because of stack overflow) because the
>> terminating condition for a recursion involved NaN.
>> >>
>> >> I've checked a little further and I have noticed that, with fast-math,
>> the square roots of negative numbers will sometimes be replaced with
>> "return undef". But only sometimes, and only the square roots (for example,
>> log(x) does not manifest this behaviour).
>> >>
>> >> It turns out a rather common way of getting the value of NaN in code
>> is to assign it the return value of sqrt(<negative number>). Because of
>> this bug, any such assignments will basically result undefined values.
>> >>
>> >> These two patches should provide a fix for this. The first is a test
>> to illustrate the situation (the negative functions will be replaced with
>> return undef in the IR), while the other is a fix for this bug.
>> >>
>> >
>> > Your patch specifically says that "sqrt(-1)" should be lowered to NaN,
>> but "float x = -1.f; sqrt(x)" should be lowered to the intrinsic, which has
>> undefined behavior according to LangRef.  This seems dubious; changing the
>> semantics of sqrt() based on the syntactic form of the source is likely to
>> cause problems.  For example, writing a trivial wrapper for sqrt (like the
>> one in libcxx for sqrtf) breaks your check.
>> >
>> > Maybe we should just kill off this transform completely?  The optimizer
>> knows how to handle a regular sqrt() call anyway.
>> >
>> > -Eli
>> >
>> > --
>> > Employee of Qualcomm Innovation Center, Inc.
>> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
>> Linux Foundation Collaborative Project
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at lists.llvm.org
>> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
> —
> Octav Zlatior
> Theobroma Systems Design und Consulting GmbH
> Seestadtstrasse 27, 1220 Wien, Austria
> Phone: +43 1 2369893-407
> http://www.theobroma-systems.com
>
>
>
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161107/a2be47a9/attachment.html>