Page MenuHomePhabricator

[CodeGen] make cbrt and fma constant (never set errno)
ClosedPublic

Authored by spatel on Nov 5 2017, 7:18 AM.

Details

Summary

Splitting cbrt and fma off from D39611, so we can deal with complex.h separately.

I've also gone ahead with a 'fix' for the fma case in CodeGenFunction::EmitBuiltinExpr(). But as noted previously, there's no test change because we were ignoring the const-ness of something that might have been non-const. Now, we're correctly checking that attribute even though there's no way it can be non-const. So if we change our minds about the fma decision, we'll do it in the right place. :)

Copying from D39611:

  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."

  1. 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.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Nov 5 2017, 7:18 AM
efriedma accepted this revision.Nov 6 2017, 12:20 PM

LGTM, but wait a couple days before you commit to see if anyone else wants to comment.

This revision is now accepted and ready to land.Nov 6 2017, 12:20 PM
spatel updated this revision to Diff 121898.Nov 7 2017, 7:34 AM

Patch updated - no code or functional changes from the last rev.

I split the <math> and <complex> test files up, so I can update those independently and not incur svn conflicts.

Based on the comments in D39611, I think we have to change this (although I'd be happy to be wrong).

  1. cbrt() can underflow and could set errno to ERANGE.
  2. fma() can overflow or underflow and set errno to ERANGE.

We could still make an exception for a GNU environment (as was drafted in D39611) if we're confident that that implementation guarantees that it won't ever set errno for these 2 functions?

cbrt() can't underflow; that's not how cube roots work. If 0<x<1, cbrt(x) > x. See also the definition of rootn() in IEEE 754 (which specifies no exceptions for n=3).

For fma(), sure, I guess we could whitelist glibc and msvcrt, if you're worried there's some unknown implementation which sets ERANGE.

spatel updated this revision to Diff 122545.Nov 10 2017, 3:42 PM

Patch updated:
cbrt() is always const. fma() is always const with GNU or Win+MSVC.

efriedma added inline comments.Nov 10 2017, 4:13 PM
lib/Sema/SemaDecl.cpp
12853 ↗(On Diff #122545)

isOSMSVCRT().

Please check the BuiltinID rather than using string compares.

spatel updated this revision to Diff 122584.Nov 11 2017, 8:42 AM
spatel marked an inline comment as done.

Patch updated:

  1. Fix predicate for detecting MSVC - isOSMSVCRT().
  2. Use switch on BuiltinID instead of string matching for "fma".
This revision was automatically updated to reflect the committed changes.