Discussion:
[PATCH] D39611: [CodeGen] make cbrt and fma constant (never set errno); document complex calls as always constant
Sanjay Patel via Phabricator via cfe-commits
2017-11-03 19:11:16 UTC
Permalink
spatel created this revision.
Herald added a subscriber: mcrosier.

These are 3 errno-related diffs raised in D39841.

1. Cube root

We're going to dismiss POSIX language like this:
"On successful completion, cbrt() returns the cube root of x. If x is NaN, cbrt() returns NaN and errno may be set to [EDOM]. "
http://pubs.opengroup.org/onlinepubs/7908799/xsh/cbrt.html

And favor an interpretation based on the C standard that says:
"Functions with a NaN argument return a NaN result and raise no floating-point exception, except where stated otherwise."

2. Floating multiply-add

The C standard is silent?
http://pubs.opengroup.org/onlinepubs/9699919799/functions/fma.html - clearly says "...then errno shall be set..."
but:
https://linux.die.net/man/3/fma - "These functions do not set errno."

Let's opt for the - hopefully common - implementation where errno is not set. Note that there's no test change because as noted in the earlier discussion, we're ignoring the const attr in CodeGenFunction::EmitBuiltinExpr(). I'll look at what needs fixing in that function next.

3. Complex

This is came up specifically for 'carg', but I'm hoping we can use 1 justification for the whole lib.

The POSIX docs all have language like this for complex calls:
"No errors are defined."
http://pubs.opengroup.org/onlinepubs/9699919799/functions/ccos.html

The lack of errno handling is allowed by the inclusion of "optionally" in the C standard definitions of all of the complex ops?
"csqrt(x + iNaN) returns NaN + iNaN and optionally raises the ‘‘invalid’’ floating-point exception, for finite x"


https://reviews.llvm.org/D39611

Files:
include/clang/Basic/Builtins.def
test/CodeGen/builtin-errno.c
test/CodeGen/libcalls-errno.c
Eli Friedman via Phabricator via cfe-commits
2017-11-03 20:32:53 UTC
Permalink
efriedma added a comment.
Post by Sanjay Patel via Phabricator via cfe-commits
"No errors are defined."
I would not trust the POSIX documentation.

carg() is an alias for atan2(), and cabs() is an alias for hypot(), so they should be marked the same way. (On glibc, cabs/hypot will set errno; not sure about carg/atan2.)

For the others transcendental functions... they can raise exceptions in some cases, so they could in theory set errno, but it looks like they don't on glibc? Not sure if that's documented anywhere, or something we should depend on.


https://reviews.llvm.org/D39611
Eli Friedman via Phabricator via cfe-commits
2017-11-03 20:35:08 UTC
Permalink
efriedma added inline comments.


================
Comment at: include/clang/Basic/Builtins.def:169
+// Disregard that 'cbrt' could set errno with a NaN input. The C standard says
+// that NaN arguments generally do not raise FP exceptions.
+BUILTIN(__builtin_cbrt , "dd", "Fnc")
----------------
Not sure adding this comment specifically for cbrt adds anything; the same could be said of a bunch of other functions, like ceil().


https://reviews.llvm.org/D39611
Hal Finkel via Phabricator via cfe-commits
2017-11-04 00:32:37 UTC
Permalink
hfinkel added a comment.
Post by Eli Friedman via Phabricator via cfe-commits
Post by Sanjay Patel via Phabricator via cfe-commits
"No errors are defined."
I would not trust the POSIX documentation.
carg() is an alias for atan2(), and cabs() is an alias for hypot(), so they should be marked the same way. (On glibc, cabs/hypot will set errno; not sure about carg/atan2.)
That seems like a bug if they set errno. The complex versions shouldn't set errno. glibc's docs say that neither hypot or set errno (although the POSIX spec allows for it in theory).
Post by Eli Friedman via Phabricator via cfe-commits
For the others transcendental functions... they can raise exceptions in some cases, so they could in theory set errno, but it looks like they don't on glibc? Not sure if that's documented anywhere, or something we should depend on.
In general, because all of these functions are well defined over the entire complex domain, they don't have errors. The "no errors defined" is likely correct.


https://reviews.llvm.org/D39611
Eli Friedman via Phabricator via cfe-commits
2017-11-04 01:47:48 UTC
Permalink
efriedma added a comment.
Post by Hal Finkel via Phabricator via cfe-commits
In general, because all of these functions are well defined over the entire complex domain, they don't have errors. The "no errors defined" is likely correct.
One, it is not true that all these functions are well-defined over the entire complex domain. For example, according to the C standard `catanh(1)` raises a divide-by-zero error.

Two, even for the functions which are defined over the entire complex domain, the result can still overflow and/or underflow, and therefore they could set errno to ERANGE. cabs() can overflow, cexp() can overflow, etc.

Three, even if we only care about the behavior of the current version of glibc, cabs() actually does modify errno. (Try `cabs(1.7e308+I*1.7e308)`).


https://reviews.llvm.org/D39611
Hal Finkel via Phabricator via cfe-commits
2017-11-04 02:28:20 UTC
Permalink
hfinkel added a comment.
Post by Eli Friedman via Phabricator via cfe-commits
Post by Hal Finkel via Phabricator via cfe-commits
In general, because all of these functions are well defined over the entire complex domain, they don't have errors. The "no errors defined" is likely correct.
One, it is not true that all these functions are well-defined over the entire complex domain. For example, according to the C standard `catanh(1)` raises a divide-by-zero error.
True. That's why I said that the statement was true in general. There are a few exceptions.
Post by Eli Friedman via Phabricator via cfe-commits
Two, even for the functions which are defined over the entire complex domain, the result can still overflow and/or underflow, and therefore they could set errno to ERANGE. cabs() can overflow, cexp() can overflow, etc.
Three, even if we only care about the behavior of the current version of glibc, cabs() actually does modify errno. (Try `cabs(1.7e308+I*1.7e308)`).
As I understand things currently, that's a bug:

In the C specification, 7.12 specifies the requirements for functions in math.h. For those functions, 7.12.1 (Treatment of error conditions) says that overflows do set ERANGE, and that it's implementation defined if the same is true for underflows. That's true for functions in math.h in general. 7.3 specifies the requirements for functions in complex.h. Here, 7.3.2 says, "An implementation may set errno but is not required to." That, as I understand it, applies to all functions in complex.h (unless otherwise noted, I presume). However, because setting errno is not required by C for functions in complex.h, when POSIX says "No errors are defined." that constrains the choice (POSIX is constraining the implementation choice allowed by C; this is not a contraction with the C specification). As a result, under POSIX, the functions in complex.h don't set errno (some, as you've noted, may raise FP exceptions, but that's another matter).

Regarding glibc-specific behavior, I do want us to be careful only to model this behavior when the target triple has -gnu. Otherwise, we should stick to the standard behavior.


https://reviews.llvm.org/D39611
Sanjay Patel via Phabricator via cfe-commits
2017-11-05 14:33:04 UTC
Permalink
spatel added a comment.
Post by Hal Finkel via Phabricator via cfe-commits
In the C specification, 7.12 specifies the requirements for functions in math.h. For those functions, 7.12.1 (Treatment of error conditions) says that overflows do set ERANGE, and that it's implementation defined if the same is true for underflows. That's true for functions in math.h in general. 7.3 specifies the requirements for functions in complex.h. Here, 7.3.2 says, "An implementation may set errno but is not required to." That, as I understand it, applies to all functions in complex.h (unless otherwise noted, I presume). However, because setting errno is not required by C for functions in complex.h, when POSIX says "No errors are defined." that constrains the choice (POSIX is constraining the implementation choice allowed by C; this is not a contraction with the C specification). As a result, under POSIX, the functions in complex.h don't set errno (some, as you've noted, may raise FP exceptions, but that's another matter).
Regarding glibc-specific behavior, I do want us to be careful only to model this behavior when the target triple has -gnu. Otherwise, we should stick to the standard behavior.
I'll split 'complex' off into its own patch then. We'll need a new attribute code for those functions. Something like:
// E -> const when -fmath-errno=0 or in a GNU (or a POSIX-compliant?) environment

I think we can detect GNU-ness with:
Context.getTargetInfo().getTriple().isGNUEnvironment()

But is that correct? If this is actually a POSIX-compliance-based constraint of the choice to not set errno, then GNU is not wide enough to cover other platforms like macOS or AIX that are (mostly?) POSIX-compliant. Also, does GNU guarantee POSIX-compliance?


https://reviews.llvm.org/D39611
Eli Friedman via Phabricator via cfe-commits
2017-11-06 21:12:45 UTC
Permalink
efriedma added a comment.

Currently, the clang driver supports three platforms which have errno-setting libc implementations: GNU/Linux (with glibc), Solaris, and Windows (with Visual Studio CRT). (BSD-based systems, including OS X, never set errno in libm, so they aren't relevant here.) As long as our markings are consistent with those targets, we should be fine.

Both MSVC and GNU libraries never set errno for cbrt() and fma(); we want to keep those assumptions in clang.

We can ignore Visual Studio for the discussion of complex.h, I guess, because MSVC doesn't support _Complex. (It has a header called complex.h, but the types aren't right, so clang won't recognize any of the functions. For reference, though, its cabs() and catanh() set errno).

I have no idea how Solaris and other Unix targets handle these functions; it's probably a bad idea to add special cases for them without testing.


https://reviews.llvm.org/D39611
Sanjay Patel via Phabricator via cfe-commits
2017-11-07 20:05:46 UTC
Permalink
spatel updated this revision to Diff 121949.
spatel retitled this revision from "[CodeGen] make cbrt and fma constant (never set errno); document complex calls as always constant" to "[CodeGen] change const-ness of complex calls".
spatel edited the summary of this revision.
spatel added a comment.

Patch updated:
I don't know if we have agreement on the behavior that we want yet, but I'm updating the patch/description/title to only deal with <complex> in this patch and taking a shot at one interpretation, so we can see what that might look like.

There are really 2 changes here:

1. We had all of <complex> marked constant all the time ('c'). I think there's agreement that can't be true by default (a platform could set errno on any of these calls based on the language in the C standard).
2. We make an exception for a GNU environment - here, the calls are always constant because the standard allows - and POSIX constrains - the errno setting behavior. This is despite the fact that glibc is known to set errno in some cases.


https://reviews.llvm.org/D39611

Files:
include/clang/Basic/Builtins.def
include/clang/Basic/Builtins.h
lib/Sema/SemaDecl.cpp
test/CodeGen/complex-builtins.c
test/CodeGen/complex-libcalls.c
Hal Finkel via Phabricator via cfe-commits
2017-11-07 23:24:38 UTC
Permalink
hfinkel added a comment.
Post by Sanjay Patel via Phabricator via cfe-commits
I don't know if we have agreement on the behavior that we want yet,
I just sent a note to the Austin group mailing list to see if the POSIX folks agree with my reading. I'll follow up.
Post by Sanjay Patel via Phabricator via cfe-commits
but I'm updating the patch/description/title to only deal with <complex> in this patch and taking a shot at one interpretation, so we can see what that might look like.
1. We had all of <complex> marked constant all the time ('c'). I think there's agreement that can't be true by default (a platform could set errno on any of these calls based on the language in the C standard).
2. We make an exception for a GNU environment - here, the calls are always constant because the standard allows - and POSIX constrains - the errno setting behavior. This is despite the fact that glibc is known to set errno in some cases.
https://reviews.llvm.org/D39611
Hal Finkel via Phabricator via cfe-commits
2017-11-10 00:28:07 UTC
Permalink
hfinkel added a comment.
Post by Hal Finkel via Phabricator via cfe-commits
Post by Sanjay Patel via Phabricator via cfe-commits
I don't know if we have agreement on the behavior that we want yet,
I just sent a note to the Austin group mailing list to see if the POSIX folks agree with my reading. I'll follow up.
They don't, so here's a summary...

1. It is not the intent of POSIX to restrict implementation choices here made available by the C specification. Oversights aside, places where POSIX intentionally restricts implementation choices allowed by C are given CX or XSI shading.

Moreover, 2.3 Error Numbers, says, "Implementations may... contain extensions or limitations that prevent some errors from occurring. The ERRORS section on each reference page specifies whether an error shall be returned, or whether it may be returned. Implementations shall not generate a different error number from the ones described here for error conditions described in this volume of IEEE Std 1003.1-2001, but may generate additional errors unless explicitly disallowed for a particular function." Because no errors are defined, and POSIX contains no specific prohibition on the complex.h functions returning errors via errno, no restriction on these functions setting errno is implied.

Thus, if C says that an implementation can set errno for functions in complex.h, POSIX does not restrict that.

2. 7.12 in C99 says that setting ERANGE on overflow is implementation-defined (same as in C90), but C99 TC3 and POSIX.1-2001 have math_errhandling, and there, when (math_errhandling & MATH_ERRNO) != 0, all overflows for functions in <math.h> set errno to ERANGE.

3. We can assume, for consistency, that the intent of the C standard is to allow math_errhandling, and thus associated requirements (e.g., the overflow requirements), to also apply to function in complex.h. G6p4 does say that there is an equivalence for floating-point exceptions (e.g., when (math_errhandling & MATH_ERREXCEPT) != 0), and so while errno is not explicitly discussed in this context, it is not unreasonable to assume this to be an oversight. It is also noted that in C99 Annex G was informative, not normative. So anything derived from Annex G is not a requirement until C11.

3. C11 DR#442 clarifies that, when Annex F is relevant, the definition of floating-point overflow in Annex F takes precedence over it being generically undefined in the absence of Annex F. Moreover, as clarified by C90 DR#025, where the type as infinities, all values are within the represented range, and so just the fact that an infinity is generated as a result implies that an overflow had occurred. Overflow only occurs for cases where a value is generated outside of the represented range for its type, such as when doing floating-point-to-integer conversions.

But, C99 has an updated definition of overflow, and it says, 7.12.1p4, "A floating result overflows if the magnitude of the mathematical result is finite but so large that the mathematical result cannot be represented without extraordinary roundoff error in an object of the specified type." Moreover, C99's Annex F says, "The ‘‘overflow’’ floating-point exception is raised whenever an infinity - or, because of
rounding direction, a maximal-magnitude finite number - is returned in lieu of a value whose magnitude is too large." While the Annex F text replaces the overflow definition, and does not mention errno explicitly, it is reasonable to assume it does not eliminate the errno-associated requirements from 7.12.1p4 intentionally.

Thus, cabs is fine to set errno, as can essentially all of the other complex.h functions.
Post by Hal Finkel via Phabricator via cfe-commits
Post by Sanjay Patel via Phabricator via cfe-commits
but I'm updating the patch/description/title to only deal with <complex> in this patch and taking a shot at one interpretation, so we can see what that might look like.
1. We had all of <complex> marked constant all the time ('c'). I think there's agreement that can't be true by default (a platform could set errno on any of these calls based on the language in the C standard).
2. We make an exception for a GNU environment - here, the calls are always constant because the standard allows - and POSIX constrains - the errno setting behavior. This is despite the fact that glibc is known to set errno in some cases.
https://reviews.llvm.org/D39611
Sanjay Patel via Phabricator via cfe-commits
2017-11-10 17:16:18 UTC
Permalink
spatel added a comment.

Thanks for the clarification!

If I'm reading this properly, we should make the same kind of change as in https://reviews.llvm.org/D39481 ('c' -> 'e') for most of complex.h. Ie, the standard allows errno-setting, and it's (unfortunately for optimization) even more clearly stated in the newer additions to the standards.

We can leave these functions as always constant ('c') because they don't actually do any math and therefore won't set errno:
cimag ( http://en.cppreference.com/w/c/numeric/complex/cimag )
creal ( http://en.cppreference.com/w/c/numeric/complex/creal )
cproj ( http://en.cppreference.com/w/c/numeric/complex/cproj )
conj (http://en.cppreference.com/w/c/numeric/complex/conj )


https://reviews.llvm.org/D39611
Hal Finkel via Phabricator via cfe-commits
2017-11-10 18:22:57 UTC
Permalink
hfinkel added a comment.
Post by Sanjay Patel via Phabricator via cfe-commits
Thanks for the clarification!
If I'm reading this properly, we should make the same kind of change as in https://reviews.llvm.org/D39481 ('c' -> 'e') for most of complex.h. Ie, the standard allows errno-setting, and it's (unfortunately for optimization) even more clearly stated in the newer additions to the standards.
cimag ( http://en.cppreference.com/w/c/numeric/complex/cimag )
creal ( http://en.cppreference.com/w/c/numeric/complex/creal )
cproj ( http://en.cppreference.com/w/c/numeric/complex/cproj )
conj (http://en.cppreference.com/w/c/numeric/complex/conj )
Sounds right to me.


https://reviews.llvm.org/D39611
Sanjay Patel via Phabricator via cfe-commits
2017-11-10 18:40:33 UTC
Permalink
spatel updated this revision to Diff 122474.
spatel added a comment.

Patch updated:
Except for cimag, cproj, conj, and creal, everything in <complex> is marked 'e' - the functions are not const if we might set errno.


https://reviews.llvm.org/D39611

Files:
include/clang/Basic/Builtins.def
test/CodeGen/complex-builtins.c
test/CodeGen/complex-libcalls.c
test/CodeGen/libcall-declarations.c
Sanjay Patel via Phabricator via cfe-commits
2017-11-17 16:13:14 UTC
Permalink
spatel added a comment.

Ping.


https://reviews.llvm.org/D39611
Eli Friedman via Phabricator via cfe-commits
2017-11-17 20:26:04 UTC
Permalink
efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D39611
Sanjay Patel via Phabricator via cfe-commits
2017-11-18 19:33:04 UTC
Permalink
This revision was automatically updated to reflect the committed changes.
Closed by commit rL318598: [CodeGen] change const-ness of complex calls (authored by spatel).

Changed prior to commit:
https://reviews.llvm.org/D39611?vs=122474&id=123467#toc

Repository:
rL LLVM

https://reviews.llvm.org/D39611

Files:
cfe/trunk/include/clang/Basic/Builtins.def
cfe/trunk/test/CodeGen/complex-builtins.c
cfe/trunk/test/CodeGen/complex-libcalls.c
cfe/trunk/test/CodeGen/libcall-declarations.c

Loading...