This is an archive of the discontinued LLVM Phabricator instance.

Use builtins in <math.h>
AbandonedPublic

Authored by michaelplatings on Sep 29 2022, 10:54 PM.

Details

Reviewers
philnik
Group Reviewers
Restricted Project
Summary

Use builtins in <math.h>

The main intended benefit of this change is that it allows libc++ to be
used with a C library that doesn't support long double.

Some platforms do not support the C library long double math functions.
For example Newlib [1].

A possible additional benefit may be better performance due to inlining.
That was the motivation for using builtins in math.h previously
e.g. D88854

Also change <complex> to use the overloaded atan2 instead of atan2l so
that the atan2 from libc++'s <math.h> is used.

The behaviour of AIX long double is unusual so it is excluded from this
change.

[1] https://github.com/mirror/newlib-cygwin/blob/58e981a5a42cd46ff52591d0394deeea9d3f01f0/newlib/libc/include/math.h#L443

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 10:54 PM
michaelplatings requested review of this revision.Sep 29 2022, 10:54 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 29 2022, 10:54 PM
philnik requested changes to this revision.Sep 30 2022, 1:53 AM
philnik added a subscriber: philnik.

What exactly is the problem here? Does libc++ not work at all when using a libc that doesn't support long double, does it error out when including <cmath> or does it result in a linker error when using the functions? Or something completely different? Assuming we want to add this configuration we also have to add a new runner for it in libcxx/utils/ci/buildkite-pipeline.yml.

This revision now requires changes to proceed.Sep 30 2022, 1:53 AM

Hi Nikolas, thanks for the quick response.

The errors look like:

include/c++/v1/math.h:794:93: error: no member named 'acosl' in the global namespace
inline _LIBCPP_INLINE_VISIBILITY long double acos(long double __lcpp_x) _NOEXCEPT {return ::acosl(__lcpp_x);}
                                                                                          ~~^

I've added a runner as you suggested and updated the tests accordingly.

Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 7:16 AM

Thanks for working on this - I made a similar change a few month back but never got around to looking at the tests (library headers only): https://reviews.llvm.org/P8292. I hit the same issue trying to use picolibc.

You patch looks a lot more complete - In the end I didn't end up using this approach and just add declarations of those functions to the newlib/picolibc headers, which was enough for the code that I was trying to build (since it didn't actually need them at link time).

One location that I believe is missing from this patch is the call to atan2l in include/complex.

Hi Nikolas, thanks for the quick response.

The errors look like:

include/c++/v1/math.h:794:93: error: no member named 'acosl' in the global namespace
inline _LIBCPP_INLINE_VISIBILITY long double acos(long double __lcpp_x) _NOEXCEPT {return ::acosl(__lcpp_x);}
                                                                                          ~~^

I've added a runner as you suggested and updated the tests accordingly.

Ok, this looks like it could be fixed another way. I'm currently working on a patch that uses builtins for all the functions in math.h. That should fix the immediate error you see and allow you to use the functions during constant evaluation (once that's implemented). The only thing that would change is that you might get a linker error instead of a compile time error when you try to pass a long double to the functions. You probably shouldn't use long doubles at all when using a libc that doesn't support it though, so I guess it should be fine. This approach would result in a lot fewer #ifndefs and would remove the need for testing another configuration. I'll try to upload the patch in the next few days.

Hi Nikolas,

I confirm that using builtins as you propose does solve the problem. And it works without explicitly setting any option, so that's even better. Yes, if you try to use a long double math function then you get an error at link time, but I think that's fine.

I look forward to your patch. If you're short of time then let me know and I can give it a go.

One location that I believe is missing from this patch is the call to atan2l in include/complex.

Thanks Alexander, you're quite right that including <complex> does produce a compile-time error without patching. So that would be another place that needs the builtin treatment, or changing to call one of the atan2 overloads. I went hunting for other headers that might need changing but didn't find any.

michaelplatings retitled this revision from [libc++] Add option to disable long double math support in libc++ to Use builtins in <math.h>.
michaelplatings edited the summary of this revision. (Show Details)

Hi Nikolas, I've updated the change to implement your suggestion.

michaelplatings removed a reviewer: ldionne.

Now clang-format approved.

I don't know where to start with the AIX test failures so guidance appreciated.

I don't know where to start with the AIX test failures so guidance appreciated.

@xingxue, will be able to help you with access to an AIX machine.

Thanks Hubert. @xingxue my email is michael dot platings at arm dot com

michaelplatings edited the summary of this revision. (Show Details)

Rebased patch on top of recent changes to math.h.

Added some exclusions for AIX in the hope that it fixes the test failure.

The previous approach of excluding the functions that AIX doesn't claim to support didn't work, so try excluding from AIX all changes to long double functions.

Sorry for the noise but this is the only way I can tell whether the change will work on AIX.

michaelplatings edited the summary of this revision. (Show Details)Oct 13 2022, 5:03 AM

That worked. @philnik can you take another look please?

This has to be coordinated with the stack starting at D135777 or this will be hell to rebase, and/or duplicate work's going to happen.

libcxx/include/math.h
794

Can't we assume that we always have the builtin available?

libcxx/include/math.h
794

I don't know. D88854, D106364 and D69806 all made sure to add the __has_builtin check. Has something changed since then? Or do you anticipate that it's only necessary for certain functions?

libcxx/include/math.h
800–802

LLVM on AIX currently only supports the case where double and long double have the same format; that may explain why there is no __builtin_acosl?

libcxx/include/math.h
800–802

Hmm. I was only speculating that there was no __builtin_acosl, but it seems that is not the case.

As @ldionne already mentioned, I've got a stack of patches to refactor math.h. I think the full stack should do everything you are trying to achieve here and more. I think you are just introducing dead code with the __has_builtin()s here. I've already got patches locally that change everything to builtins. Because of that I think this patch is redundant. Still, thanks for working on the patch. Do you want me to tag you on related patches I upload, so you can track development?

michaelplatings abandoned this revision.Oct 18 2022, 5:21 AM

Hi @philnik, I'm abandoning this patch in anticipation of you changing all the math.h functions to use builtins.

The main thing you can learn from this patch is that you'll run into issues with AIX long double. I haven't been able to identify exactly which functions are problematic but I'll let you know if I do.

Do you want me to tag you on related patches I upload, so you can track development?

Yes please.