Page MenuHomePhabricator

[CodeGen] map sqrt libcalls to llvm.sqrt when errno is not set
ClosedPublic

Authored by spatel on Oct 23 2017, 1:49 PM.

Details

Summary

The LLVM sqrt intrinsic definition changed with:
D28797
...so we don't have to use any relaxed FP settings other than errno handling.

This patch sidesteps a question raised in PR27435:
https://bugs.llvm.org/show_bug.cgi?id=27435

Is a programmer using __builtin_sqrt() invoking the compiler's intrinsic definition of sqrt or the mathlib definition of sqrt?

But we have an answer now: the builtin should match the behavior of the libm function including errno handling.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Oct 23 2017, 1:49 PM
efriedma edited edge metadata.Oct 23 2017, 2:50 PM

The gcc documentation says "GCC includes built-in versions of many of the functions in the standard C library. These functions come in two forms: one whose names start with the __builtin_ prefix, and the other without. Both forms have the same type (including prototype), the same address (when their address is taken), and the same meaning as the C library functions". And gcc specifically preserves the stated semantics. Given that, I'm not sure it makes sense for us to try to redefine __builtin_sqrt() just because it's convenient.

Note that this reasoning only applies if the user hasn't specified any fast-math flags; under -ffinite-math-only, we can assume the result isn't a NaN, and therefore we can use llvm.sqrt.*. (The definition of llvm.sqrt.* changed in https://reviews.llvm.org/D28797; I don't think we ever updated clang to take advantage of this).

If we really need a name for the never-sets-errno sqrt, we should probably use a different name, e.g. __builtin_ieee_sqrt().

The gcc documentation says "GCC includes built-in versions of many of the functions in the standard C library. These functions come in two forms: one whose names start with the __builtin_ prefix, and the other without. Both forms have the same type (including prototype), the same address (when their address is taken), and the same meaning as the C library functions". And gcc specifically preserves the stated semantics. Given that, I'm not sure it makes sense for us to try to redefine __builtin_sqrt() just because it's convenient.

Note that this reasoning only applies if the user hasn't specified any fast-math flags; under -ffinite-math-only, we can assume the result isn't a NaN, and therefore we can use llvm.sqrt.*. (The definition of llvm.sqrt.* changed in https://reviews.llvm.org/D28797; I don't think we ever updated clang to take advantage of this).

If we really need a name for the never-sets-errno sqrt, we should probably use a different name, e.g. __builtin_ieee_sqrt().

Thanks for the explanation and link. Let me know if I've gone wrong:

  1. We don't want to convert clang math builtins to llvm intrinsics because builtins are supposed to be exactly equivalent to C library functions (including setting errno).
  2. LLVM intrinsics should be equivalent to C library functions except that they don't set errno (but this is currently wrong in some cases, and D28335 would fix that).
  3. Therefore, the existing code in this file that is converting 'pow' and other builtin calls to intrinsics is correct for now, but only because 2 wrongs made it right? :)

Working my way through the stack: does the sqrt LangRef change mean we can revert rL265521? What allows us to transform any of those libm calls that set errno to vectors in the first place?

I'm even more confused than usual, but if I can find a way to untangle this mess, I'll try to start making patches.

I think you're understanding the semantics correctly.

For r265521, look again at the implementation of llvm::checkUnaryFloatSignature; if "I.onlyReadsMemory()" is true, we somehow proved the call doesn't set errno (mostly likely because we know something about the target's libm).

hfinkel edited edge metadata.Oct 26 2017, 3:46 PM

I think you're understanding the semantics correctly.

For r265521, look again at the implementation of llvm::checkUnaryFloatSignature; if "I.onlyReadsMemory()" is true, we somehow proved the call doesn't set errno (mostly likely because we know something about the target's libm).

Right. Either by target default, or because the user passed -fno-math-errno (or something that implies it, such as -ffast-math), we mark the functions as readonly/readnone because they won't write to errno.

Working my way through the stack: does the sqrt LangRef change mean we can revert rL265521?

Yes, maybe. We can now form the intrinsics from the library calls so long as we only care about the result (and not the value of errno), even if the input is negative or would otherwise generate a NaN (as the intrinsic no longer as UB in those situations). However, we still need to know that the potential value of errno is of no interest. I'm not sure how we know that without some particular modeling.

What allows us to transform any of those libm calls that set errno to vectors in the first place?

I'm even more confused than usual, but if I can find a way to untangle this mess, I'll try to start making patches.

spatel updated this revision to Diff 120902.Oct 30 2017, 3:37 PM

Patch updated:
As suggested, I've morphed this patch to just handle sqrt libcalls based on the updated LLVM intrinsic definition.

I was going to include the builtins too, but that exposes another bug (marked here with FIXME) - the builtins are all defined 'const'. Therefore, they can never set errno (unless I'm still misunderstanding). So I think we are wrongly currently turning those into libcalls marked 'readnone'.

We could wrongly turn those into intrinsics in this patch if that seems better? :)

This does make me curious about the use-case of libm-equivalent builtins. If they are exactly identical to libm (including errno behavior), then why are they needed in the first place?

I was going to include the builtins too, but that exposes another bug (marked here with FIXME) - the builtins are all defined 'const'.

Probably just need to change c->e in Builtins.def?

This does make me curious about the use-case of libm-equivalent builtins. If they are exactly identical to libm (including errno behavior), then why are they needed in the first place?

It gets treated differently under -fno-builtin/-std=c89.

I was going to include the builtins too, but that exposes another bug (marked here with FIXME) - the builtins are all defined 'const'.

Probably just need to change c->e in Builtins.def?

Yes - at least that made sqrt behave like I expected. So I think it's really just a question of what order and combination that we want to fix this in:

  1. Just the sqrt libcalls in this patch.
  2. Fix both sqrt libcalls and __builtin_sqrt in this patch.
  3. Fix all libm libcalls and builtins simultaneously.
efriedma accepted this revision.Oct 31 2017, 11:45 AM

Let's do this one step at a time; first this patch, then fix the __builtin_* functions to use "e", then add all the missing cases CodeGenFunction::EmitBuiltinExpr.

This patch LGTM, assuming you fix the commit message (and the title on Phabricator) to properly describe the change.

This revision is now accepted and ready to land.Oct 31 2017, 11:45 AM

Let's do this one step at a time; first this patch, then fix the __builtin_* functions to use "e", then add all the missing cases CodeGenFunction::EmitBuiltinExpr.

Sounds good.

This patch LGTM, assuming you fix the commit message (and the title on Phabricator) to properly describe the change.

Sure - I didn't know if we prefer to leave the title for the sake of email thread continuity on the list or not, but I'll update that now.

spatel retitled this revision from [CodeGen] __builtin_sqrt should map to the compiler's intrinsic sqrt function to [CodeGen] map sqrt libcalls to llvm.sqrt when errno is not set.Oct 31 2017, 12:45 PM
spatel edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.