----- Original Message ----- > > On Thu, Sep 12, 2013 at 3:55 PM, Hal Finkel < [email protected] > > wrote: > > > > > > > ----- Original Message ----- > > > > On Thu, Sep 12, 2013 at 2:44 PM, Hal Finkel < [email protected] > > > wrote: > > > > > > > > > > > > ----- Original Message ----- > > > > > > On Thu, Sep 12, 2013 at 2:03 PM, Hal Finkel < [email protected] > > > > 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
