f32/f64 sqrt has undefined behaviour for values less than -0.0

[thestinger]

http://llvm.org/docs/LangRef.html#llvm-sqrt-intrinsic

[thestinger]

cc @bjz, @jensnockert

[auroranockert]

It is correct that this is an issue, but I think the solution is wrong.

We should not break with IEEE 754 unless we have really good reason. The correct thing imho is to essentially to return NaN, but we should double-check the standard.

[thestinger]

@jensnockert: The POSIX/C solution is to set errno to EDOM and set the floating point exception to FE_INVALID. I don't know if anything is specified by IEEE754.

[auroranockert]

POSIX/C does not follow IEEE 754, it allows for different formats of floating-point, and doesn't require NaN. That is why it has an additional way to signal out-of-domain. IEEE 754 was not ubiquitous when they were designed.

From the POSIX standard

Finite values of x < -0, a domain error shall occur, and either a NaN (if supported), or an implementation-defined value shall be returned.

But since we can assume IEEE 754 in the standard library (there are no other floating-point standards that are popular anymore) I think we should just return NaN.

From IEEE 754-2008

7.2 Invalid operation 7.2.0
…, the default result of an operation that signals the invalid operation exception shall be a quiet NaN that should provide some diagnostic information (see 6.2). These operations are:
…
g) squareRoot if the operand is less than zero

I believe that the POSIX workaround will be surprising to others, since we have an in-band signalling mechanism that is standardized. This is the same in-band mechanism we use for example overflow and division by zero for floating-point operands.

[thestinger]

Ah, okay, that's a much simpler way to do it.

[thestinger]

@jensnockert: would we just be returning 0.0 / 0.0? or is there something special you have to do for the diagnostic part?

[auroranockert]

That should be fine.

[thestinger]

Wouldn't we have to also make sure the floating point environment exception is set properly? Or are we not worrying about that?

[auroranockert]

0 / 0 throws the same exception as sqrt of something negative (invalid operation), so it should be fine. But if we ever expose floating-point exceptions, then we would probably need to check the f32/f64 modules for similar issues.

[thestinger]

@jensnockert: I'll get around to sending in a fix like this to the standard library then: https://github.com/thestinger/rust-core/commit/b5cdd2c10952527f100bc439b1ecb993dface0d1

[auroranockert]

Looks awesome.

[thestinger]

http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-November/067450.html

[DanielKeep]

The following is true as of right now on the playpen:

assert_eq!((-1.0f32).sqrt(), 0.0f32);

sqrt of a negative number should return NaN.

[thestinger]

@DanielKeep: It's undefined behaviour. The returned result is arbitrary so it will change based on optimizations. It will crash / corrupt memory in some cases. Even if it happened to return NaN in any test cases we tried, this would still be a bug.