Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

'__builtin_fpclassify' and '__builtin_inf' (and probably others) lack sema and codegen support #6552

Open Quuxplusone opened 14 years ago

Quuxplusone commented 14 years ago
Bugzilla Link PR6083
Status NEW
Importance P normal
Reported by Maurice Gittens (mainmanmauricio@gmail.com)
Reported on 2010-01-19 10:39:42 -0800
Last modified on 2018-10-25 20:11:53 -0700
Version trunk
Hardware PC Linux
CC benny.kra@gmail.com, chris@bubblescope.net, dgregor@apple.com, dsaritz@gmail.com, evan@chromium.org, jyasskin@google.com, llvm-bugs@lists.llvm.org, nlewycky@google.com, richard-llvm@metafoo.co.uk
Fixed by commit(s)
Attachments builtin_fpclassify_codegen.patch (5495 bytes, text/plain)
codegen_builtin_fpclassify.patch (2902 bytes, text/plain)
Blocks
Blocked by
See also
It seems that the gcc builtin '__builtin_fpclassify' is required
by boost while not available in clang++.
Quuxplusone commented 14 years ago

Which Boost header is using __builtin_fpclassify?

Quuxplusone commented 14 years ago
(In reply to comment #1)
> Which Boost header is using __builtin_fpclassify?
>

The include stack at the point of the error testifies:

In file included from boost-test.cpp:112:
In file included from /usr/include/boost/bimap.hpp:13:
In file included from /usr/include/boost/bimap/bimap.hpp:61:
In file included from /usr/include/boost/bimap/detail/bimap_core.hpp:38:
In file included from /usr/include/boost/bimap/relation/mutant_relation.hpp:26:
In file included from /usr/include/boost/functional/hash/hash.hpp:15:
In file included from /usr/include/boost/functional/detail/hash_float.hpp:25:
In file included from
/usr/include/boost/functional/detail/float_functions.hpp:9:
In file included from /usr/include/boost/config/no_tr1/cmath.hpp:21:
/usr/include/c++/4.4.1/cmath:499:14: error: use of undeclared identifier
'__builtin_fpclassify'
return __builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL,

So, I guess boost is including cmath.

Cheers,
Maurice
Quuxplusone commented 14 years ago
The only interesting thing I see about this is here:
http://archives.free.net.ph/message/20080523.164003.a971bed4.ja.html

Apparently this is used like this:

#define FP_NAN 1
#define FP_INF 2
#define FP_INFINITE 2
#define FP_NORMAL 3
#define FP_SUBNORMAL 4
#define FP_ZERO 5
__builtin_fpclassify(FP_NAN, FP_INFINITE, FP_NORMAL, FP_SUBNORMAL, FP_ZERO, (X))
Quuxplusone commented 14 years ago
With this patch, we should be able to parse the header:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20100118/026684.html

However, we'll still reject the code if something actually tries to codegen the
builtin.  Here's the GCC implementation of this feature if anyone is interested:
http://gcc.gnu.org/ml/gcc-patches/2008-05/msg01255.html
Quuxplusone commented 14 years ago

I've added Sema support for __builtin_fpclassify in r96291. I'll post a codegen patch soon.

Quuxplusone commented 14 years ago

Attached builtin_fpclassify_codegen.patch (5495 bytes, text/plain): first shot at codegen support

Quuxplusone commented 14 years ago

Hey Benjamin, it's probably best to email the patch to cfe-commits to get someone to review it, not many people watch bugzilla.

Quuxplusone commented 14 years ago

WebKit uses isfinite(), which also requires builtin support.

Quuxplusone commented 14 years ago

This isn't actually Boost-related, so it doesn't block Boost.

Quuxplusone commented 14 years ago

_Bug 6987 has been marked as a duplicate of this bug._

Quuxplusone commented 14 years ago

Just to confirm, this isn't blocking boost.

It is however blocking including on g++ 4.4.x and 4.5.x.

Quuxplusone commented 14 years ago

isinf is implemented in r103166

Quuxplusone commented 14 years ago

__builtin_isfinite is implemented in r103168

Quuxplusone commented 14 years ago
I added some todo's for isinf_sign and isnormal in r103169.  I don't plan to
implement these, and they are need fpclassify.  Benjamin, your patch looks
basically right, but please use the algorithms and APFloat methods mentioned in
the comment I added.  For fpclassify, I recommend actually emitting a branching
structure, like this:

if (x == 0.0) {
  res = ZERO;
} else {
  xabs = fabs(x);
  if (xabs == infinity)
    res = INFINITY;
  else if (xabs == xabs)
    res = NORMAL;
  else
    res = NAN;
}

Emitting actual branches is good for the branch predictor, codegen can decide
to fold together the blocks if it wants to.
Quuxplusone commented 14 years ago

__builtin_isnormal is implemented in r104118.

Quuxplusone commented 14 years ago

Attached codegen_builtin_fpclassify.patch (2902 bytes, text/plain): CodeGen __builtin_fpclassify implementation

Quuxplusone commented 14 years ago
(In reply to comment #16)
> I created an implementation of __builtin__fpclassify the way Chris Lattner
> described.

Very nice, commited as r105936.
Quuxplusone commented 14 years ago
On linux, linking a trivial program like:

int main(void)
{ return __builtin_fpclassify(1,2,3,4,5,1.0); }

produces the error:

undefined reference to 'fabs'

Which goes away if I add -lm. However, -lm is not required with gcc. This does
not occur on Darwin.
Quuxplusone commented 14 years ago

Just a FYI. gcc rejects calls to __builtin_fpclassify if any of the first 5 parameters (the constant list) are non-constant. Of course there is nothing wrong with allowing more values, I just wanted to check this case had been considered, as gcc purposefully rejects it.

Quuxplusone commented 13 years ago

Comment 18 is caused by the issue in bug 9019.