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

Mon Nov 7 03:09:15 PST 2016

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.

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.

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-commits/Week-of-Mon-20140609/220450.html>
> http://lists.llvm.org/pipermail/llvm-dev/2014-September/077137.html <http://lists.llvm.org/pipermail/llvm-dev/2014-September/077137.html>
> http://llvm.org/bugs/show_bug.cgi?id=21048 <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 <mailto: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 <mailto: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 <mailto:llvm-commits at lists.llvm.org>
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161107/d0cae760/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 842 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161107/d0cae760/attachment.sig>