Page MenuHomePhabricator

CodeGen: Emit sqrt intrinsic from __builtin_sqrt
Needs ReviewPublic

Authored by tstellarAMD on Mar 19 2015, 2:51 PM.

Details

Summary

We need to add a check for x < -0.0 before the intrinsic, because it has undefined behavior in this case.

Diff Detail

Repository
rL LLVM

Event Timeline

tstellarAMD retitled this revision from to CodeGen: Emit sqrt intrinsic from __builtin_sqrt.
tstellarAMD updated this object.
tstellarAMD edited the test plan for this revision. (Show Details)
tstellarAMD set the repository for this revision to rL LLVM.
tstellarAMD added a subscriber: Unknown Object (MLST).
tstellarAMD edited subscribers, added: Unknown Object (MLST); removed: Unknown Object (MLST).
hfinkel edited edge metadata.Mar 19 2015, 2:59 PM

To play devil's advocate: I don't understand what you're trying to fix here. So the intrinsic has undefined behavior if x < -0.0; that's the definition of our intrinsic.

Regardless, you don't need to insert the extra code if we're in NoNaNs mode.

To play devil's advocate: I don't understand what you're trying to fix here. So the intrinsic has undefined behavior if x < -0.0; that's the definition of our intrinsic.

The check is there to avoid the undefined behavior. My understanding is that __builtin_sqrtf is supposed to return NaN for x < -0.0, so we need to handle that case specially if we emit the intrinsic.

Regardless, you don't need to insert the extra code if we're in NoNaNs mode.

To play devil's advocate: I don't understand what you're trying to fix here. So the intrinsic has undefined behavior if x < -0.0; that's the definition of our intrinsic.

The check is there to avoid the undefined behavior. My understanding is that __builtin_sqrtf is supposed to return NaN for x < -0.0, so we need to handle that case specially if we emit the intrinsic.

Right, I just want to understand what "supposed to" means? (what GCC does?)

Regardless, you don't need to insert the extra code if we're in NoNaNs mode.

To play devil's advocate: I don't understand what you're trying to fix here. So the intrinsic has undefined behavior if x < -0.0; that's the definition of our intrinsic.

The check is there to avoid the undefined behavior. My understanding is that __builtin_sqrtf is supposed to return NaN for x < -0.0, so we need to handle that case specially if we emit the intrinsic.

Right, I just want to understand what "supposed to" means? (what GCC does?)

No, it's what libm does. Aren't all the __builtin prefixed library calls supposed to be identical to the library call except that they don't set errno?

Regardless, you don't need to insert the extra code if we're in NoNaNs mode.

To play devil's advocate: I don't understand what you're trying to fix here. So the intrinsic has undefined behavior if x < -0.0; that's the definition of our intrinsic.

The check is there to avoid the undefined behavior. My understanding is that __builtin_sqrtf is supposed to return NaN for x < -0.0, so we need to handle that case specially if we emit the intrinsic.

Right, I just want to understand what "supposed to" means? (what GCC does?)

No, it's what libm does. Aren't all the __builtin prefixed library calls supposed to be identical to the library call except that they don't set errno?

At least in practice, no. Here's the problem: For all of the math builtins, it is possible that the target will naively support the operation, and so you'll get the underlying operation, this is the same as what the library function would do except that errno is untouched. However, if the target has no such support, then, you do get a call to the library function, and errno might be changed. Regardless, for all such builtins, we mark the corresponding call as readonly/readnone, as if it will never set errno (and, thus, if it does, we can miscompile code by reordering the call with some other call, to open() for example, that sets errno in a way we care about). Thus, the fact that the builtin call does not set errno is not a structural guarantee but a contract: the user promises never to call the builtin with an argument such that the library call would set errno. Otherwise, the behavior is undefined. In return, the compiler will optimize as though the call will never set errno.

IMHO, what we really need here is better documentation on this point.

Regardless, you don't need to insert the extra code if we're in NoNaNs mode.