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

Mon Nov 7 11:18:03 PST 2016

----- Original Message -----

> From: "Michael Kuperstein via llvm-commits"
> <llvm-commits at lists.llvm.org>
> To: "Octav Zlatior" <octav.zlatior at theobroma-systems.com>
> Cc: "llvm-commits" <llvm-commits at lists.llvm.org>
> Sent: Monday, November 7, 2016 12:57:04 PM
> Subject: Re: fast-math: fix a bug related to square roots of
> negatives

> 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.
+1 

-Hal 

> > 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
> 

> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-- 

Hal Finkel 
Lead, Compiler Technology and Programming Languages 
Leadership Computing Facility 
Argonne National Laboratory 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161107/6d4d09d7/attachment.html>