This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Use builtin in isinf/isnormal/fpclassify/isfinite
AbandonedPublic

Authored by qiucf on Feb 5 2021, 4:49 AM.

Details

Reviewers
EricWF
TokarIP
ldionne
Group Reviewers
Restricted Project
Summary

In bootstrap testing on Power9 target with libcxx enabled, build would fail because glibc uses __builtin_types_compatible_p in definition of these macros but the builtin is not available in C++ mode. And glibc assumes:

/* Since __builtin_isinf_sign is broken for float128 before GCC 7.0,
   use the helper function, __isinff128, with older compilers.  This is
   only provided for C mode, because in C++ mode, GCC has no support
   for __builtin_types_compatible_p (and when in C++ mode, this macro is
   not used anyway, because libstdc++ headers undefine it).  */

So we need another way to do the implementation, just like rG767eadd78.

Diff Detail

Event Timeline

qiucf requested review of this revision.Feb 5 2021, 4:49 AM
qiucf created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2021, 4:49 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Feb 5 2021, 8:35 AM
This revision is now accepted and ready to land.Feb 5 2021, 8:35 AM

FYI https://reviews.llvm.org/D88854 does this for more builtins/functions

FYI https://reviews.llvm.org/D88854 does this for more builtins/functions

That revision wasn't easily discoverable. I added the LLVM Monorepo tag to it and now it's in the review queue.

I looked at the other review and it does seem to supersede this one. I would suggest we go ahead with the other one once CI passes, and so we should be able to just abandon this one. I'll let the two authors decide if they want to do that or something else (e.g. ship this one and rebase the other one).

qiucf added a comment.Feb 5 2021, 6:07 PM

FYI https://reviews.llvm.org/D88854 does this for more builtins/functions

Thanks for the information! I did not notice another patch about this exists. I can abandon this after that is committed.

The other patch has been landed, I think we can abandon this. Thanks!

qiucf abandoned this revision.Feb 13 2021, 3:08 AM