[PATCH] Restore the sqrt -> llvm.sqrt mapping in fast-math mode

[Hal Finkel]

----- Original Message -----
> 
> On Thu, Sep 12, 2013 at 3:55 PM, Hal Finkel < hfinkel at anl.gov >
> wrote:
> 
> 
> 
> 
> 
> 
> ----- Original Message -----
> > 
> > On Thu, Sep 12, 2013 at 2:44 PM, Hal Finkel < hfinkel at anl.gov >
> > wrote:
> > 
> > 
> > 
> > 
> > 
> > ----- Original Message -----
> > > 
> > > On Thu, Sep 12, 2013 at 2:03 PM, Hal Finkel < hfinkel at anl.gov >
> > > wrote:
> > > 
> > > 
> > > 
> > > 
> > > Hello,
> > > 
> > > Please review the attached patch which restores the libm sqrt* ->
> > > @llvm.sqrt* mapping, but only in fast-math mode (specifically,
> > > when
> > > the UnsafeFPMath or NoNaNsFPMath CodeGen options are enabled).
> > > The
> > > @llvm.sqrt* intrinsics have slightly different semantics from the
> > > libm call, specifically, they are undefined when given a non-zero
> > > negative number (the libm calls will always return NaN for any
> > > negative number).
> > > 
> > > This mapping was removed in r100613, and replaced with a TODO,
> > > but
> > > at
> > > that time the fast-math flags were not yet implemented. Now that
> > > we
> > > have these, restoring this mapping is important because it will
> > > enable autovectorization of sqrt calls in loops (at least in
> > > fast-math mode).
> > > 
> > > 
> > > 
> > > 
> > > This is dangerous, if LangRef is actually correct. People don't
> > > associate -ffast-math with "my program will crash at random". :)
> > > Of
> > > course, LangRef is probably overstating the issue.
> > 
> > I agree, and the LangRef does indeed say "undefined behavior", but
> > I
> > assume that should really mean, "returns an undefined value." Do
> > you
> > agree?
> > 
> > 
> > 
> > Well, if we map llvm.sqrt to sqrt and sqrt sets errno, we really do
> > mean "undefined behavior"... or at least something more that
> > "returns an undefined value".
> 
> Agreed, but this possibility-of-setting-errno problem exists for all
> LLVM libm-style intrinsics, and so also exists for pow() [for which
> we currently do this exact kind of replacement whenever
> -fmath-errno=0]. So, FWIW, this is not without precedent.
> 
> 
> > 
> > 
> > 
> > 
> > > 
> > > 
> > > That said, there's actually a general issue here: if we map the
> > > LLVM
> > > intrinsics to libc functions, and the libc functions set errno,
> > > we
> > > could break code that depends on errno for non-math calls (e.g.
> > > fopen().)
> > 
> > Perhaps, but I'm not changing that here. For one thing, if the
> > mapping does, in effect, sqrt -> llvm.sqrt -> sqrt, and only when
> > -fmath-errno=0. Are you worried about cases where the libm
> > functions
> > actually do set errno (even though we have -fmath-errno=0)?
> > 
> > 
> > 
> > 
> > I'm not sure our implementation of -fno-math-errno is safe:
> > according
> > to the gcc manual, it isn't equivalent to marking the math
> > functions
> > with attribute((const)). (For example, the gcc manual's definition
> > allows transforming a call to sqrt() into the SSE sqrt instruction,
> > but it doesn't allow hoisting a call to sqrt out of arbitrary loops
> > on a machine where the sqrt() call could set errno.)
> 
> I'm sure it is not safe, for the very reason that you highlight.
> Nevertheless, this is a long-standing problem, affecting the
> implementation of -fmath-errno=0 on all systems for which libm math
> functions actually do set errno, and will require a general solution
> (fairly orthogonal to this patch).
> 
> I recommend that we:
> 
> 1. Commit this change (so that we can autovectorize calls to sqrt()).
> 2. Have a discussion about how to actually solve this problem: I
> think that it involves making a specific function attribute for
> setting errno, and teaching the alias analysis infrastructure to do
> something sensible with it.
> 
> Okay.

r190646, Thanks!

Amusingly enough, I think that this change actually helps this situation: with -fmath-errno=0, sqrt gets marked as readnone, while at least the intrinsic is only marked readonly.

 -Hal

> 
> 
> -Eli

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory