This is an archive of the discontinued LLVM Phabricator instance.

[libc++][math.h] Use builtins for all the functions
ClosedPublic

Authored by philnik on Oct 20 2022, 4:42 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Commits
rG650da4a690f9: [libc++][math.h] Use builtins for all the functions
Summary

This allows compiling libc++, even when the C library doesn't support floating point math.

Diff Detail

Event Timeline

philnik created this revision.Oct 20 2022, 4:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 4:42 PM
philnik requested review of this revision.Oct 20 2022, 4:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 4:42 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Looks good but you're hitting the same problem I did with AIX.

I suspect that __builtin_ldexpl is the one that's causing the trouble but haven't been able to prove it. @xingxue did give me access to an AIX machine but unfortunately it's under such heavy load already that it's not really usable. The path I took was to not use long double builtins on AIX ¯\_(ツ)_/¯

philnik edited the summary of this revision. (Show Details)Oct 30 2022, 2:14 PM
philnik updated this revision to Diff 471874.Oct 30 2022, 2:29 PM
  • Rebased
  • Added AIX workaround
philnik updated this revision to Diff 471875.Oct 30 2022, 2:30 PM

Add semicolon

philnik retitled this revision from [libc++] Use builtins for all the functions in math.h to [libc++][math.h] Use builtins for all the functions.Nov 1 2022, 2:33 PM
philnik updated this revision to Diff 472409.Nov 1 2022, 2:50 PM

Fix rebase

ldionne accepted this revision.Nov 3 2022, 9:10 AM
ldionne added a subscriber: daltenty.

LGTM but please ping me if you need major changes to make the CI pass.

libcxx/include/math.h
681

Is there a reason why this builtin doesn't work correctly on AIX?

@daltenty @xingxue Can you get someone to fix this? When we try to make those constexpr, things could start failing pretty badly on AIX if we're not using builtins.

This revision is now accepted and ready to land.Nov 3 2022, 9:10 AM
xingxue added inline comments.Nov 10 2022, 8:32 AM
libcxx/include/math.h
681

The issue is that AIX implements math functions frexpl(), ldexpl(), and modfl() as 128-bit long double but for other math functions, AIX libm has 64-bit long double wrappers. Currently, the Clang compiler on AIX supports 64-bit long double only and therefore, frexpl(), ldexpl(), and modfl() would seg-fault. We are looking into a fix.

xingxue added inline comments.Nov 14 2022, 2:56 PM
libcxx/include/math.h
681

Patch D137986 has been created for the compiler to map __builtin_frexpl(), __builtin_ldexpl(), and __builtin_modfl() to double version functions frexp(), ldexp(), and modf() in 64-bit long double mode.

ldionne added inline comments.Nov 20 2022, 8:18 AM
libcxx/include/math.h
1004–1005

We also define hypot inside <cmath>. Do you know why?

ldionne added inline comments.Nov 21 2022, 8:18 AM
libcxx/include/math.h
1004–1005

Per discussion just now, the <cmath> version is a 3-arg overload and only available in C++17.

ldionne added inline comments.Nov 23 2022, 7:28 AM
libcxx/include/math.h
1294–1296

I know this is not the main purpose of this patch, but this doesn't help us if we want to support systems where e.g. tgamma(double) is not provided. Indeed, with this patch, we end up calling tgama(double) below, which is ambiguous between tgamma(float) and tgamma(long double), since there is no tgamma(double).

philnik added inline comments.Nov 23 2022, 8:58 AM
libcxx/include/math.h
1294–1296

I'm planning to add double overloads, but that will be a later patch.

xingxue added inline comments.Nov 24 2022, 8:16 AM
libcxx/include/math.h
681

@philnik @ldionne The compiler on the AIX buildbot for CI has been updated with patch D137986. We should be able to remove the workaround for AIX in math.h now.

ldionne added inline comments.Nov 24 2022, 8:58 AM
libcxx/include/math.h
681

Nice, thanks!

philnik updated this revision to Diff 478639.Nov 29 2022, 10:48 AM
philnik marked 5 inline comments as done.
  • Remove AIX workaround
  • Rebased
philnik marked an inline comment as done.Nov 29 2022, 10:48 AM
This revision was landed with ongoing or failed builds.Nov 29 2022, 3:50 PM
This revision was automatically updated to reflect the committed changes.