Differential D39204
[CodeGen] map sqrt libcalls to llvm.sqrt when errno is not set Authored by spatel on Oct 23 2017, 1:49 PM.
Details The LLVM sqrt intrinsic definition changed with: This patch sidesteps a question raised in PR27435: 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 Event TimelineComment Actions 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(). Comment Actions Thanks for the explanation and link. Let me know if I've gone wrong:
Comment Actions 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. Comment Actions 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). Comment Actions 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. 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.
Comment Actions Patch updated: 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? Comment Actions
Probably just need to change c->e in Builtins.def?
It gets treated differently under -fno-builtin/-std=c89. Comment Actions 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:
Comment Actions 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. Comment Actions Sounds good.
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. |