Page MenuHomePhabricator

[TargetLowering] Android has sincos functions
ClosedPublic

Authored by john.brawn on Sep 13 2018, 4:27 AM.

Details

Summary

Since Android API version 9 the Android libm has had the sincos functions, so they should be recognised as libcalls and sincos optimisation should be applied.

Diff Detail

Repository
rL LLVM

Event Timeline

john.brawn created this revision.Sep 13 2018, 4:27 AM
xbolva00 added inline comments.
lib/CodeGen/TargetLoweringBase.cpp
165 ↗(On Diff #165246)

I think there should be SDK version.. sdk 28.

john.brawn added inline comments.Sep 13 2018, 5:11 AM
lib/CodeGen/TargetLoweringBase.cpp
165 ↗(On Diff #165246)

I do actually mean 9 here. It looks like sincos was added by https://android.googlesource.com/platform/bionic/+/ddd235bd9c264f08dee7887e210d61ca2351cf86 and https://android.googlesource.com/platform/bionic/+/9946750609c858dad0150da55645c4331392cf0d which first appeared in Android 2.3 which is API version 9.

xbolva00 added inline comments.Sep 13 2018, 5:31 AM
lib/CodeGen/TargetLoweringBase.cpp
165 ↗(On Diff #165246)

Ah sorry, I thought you mean Android 9 Pie :D

pirama accepted this revision.Sep 14 2018, 10:00 AM
pirama added subscribers: enh, danalbert.
This revision is now accepted and ready to land.Sep 14 2018, 10:00 AM
This revision was automatically updated to reflect the committed changes.

Someone just pointed out to me that even in Android P [1], libm's sincos() just called sin() followed by cos() so this optimization won't be beneficial. It was changed in March 2018 [2] but is not a part of any release yet.

Unless there's any objection, let's revert this change and reintroduce it at a future time, enabling it from the next Android release forward.

[1] https://android.googlesource.com/platform/bionic/+/pie-release/libm/sincos.c
[2] https://android-review.googlesource.com/c/platform/bionic/+/681682/

I just wanted to follow up here.

tl;dr - The patch is actually ok as-is, and does not need to be reverted.

At worst, this results in an extra call to sincos() (that then calls sin() followed by cos()) on older Android releases. For all newer releases, it will indeed use the optimized path. The only drawback to this patch is that it prevents new toolchains from being used to compile older versions of the Android platform (i.e. the core operating system code). This happens all the way up to Android Pie. Since AOSP has moved on, however, I don't feel that this is a big concern. Partners generally don't update toolchains for previously released OS versions, and newer OS versions (with newer toolchains) don't experience this problem.