-
Notifications
You must be signed in to change notification settings - Fork 12.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[builtins] Convert more int to fp functions to use common implementation #67540
Conversation
Builds in llvm#66903, converting the rest of the low-hanging fruit to use the common implementation.
30951ff
to
475c6cb
Compare
Ping for review. I've rebased now that #66903 has landed and additionally can demonstrate equivalence using Alive2 (caveat below):
The caveat is that alive2 won't show equivalence for the functions that take i128 as an input where |
The changes look good to me, thank you. |
This seems to have broken __floatuntitf for x86_64 (noticed after rebasing #68132 on top of this). |
__floattitf(1) is returning 0.0 instead of 1.0 with this change. |
I found the underlying issue which is not a regression from this chage, just a failed conflict resultion on my side. Apologies for the noise. |
This patch badly broke the Solaris/sparcv9 buildbot which has 128-bit long double, but no |
@@ -16,6 +16,10 @@ | |||
#include "fp_lib.h" | |||
#include "int_lib.h" | |||
|
|||
#define SRC_U128 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one way to fix the solaris error would be to lift
#if defined(CRT_HAS_TF_MODE)
before this include since it also requires __int128.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or adding a #ifdef CRT_HAS_128BIT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works indeed. floattitf.c
needs the same patch. It's unfortunate, however, that int_to_fp.h
isn't more robust, unconditionally using types whose existance isn't guaranteed everywhere.
69660cc broke the Solaris/sparcv9 buildbot https://lab.llvm.org/staging/#/builders/12/builds/264: `compiler-rt/lib/builtins/int_to_fp.h` unconditionally uses `*int128_t` which don't exist on 32-bit SPARC. As suggested in llvm#67540, this patch fixes this by moving the `CRT_HAS_TF_MODE` guard up which does the necessary checks. Tested on `sparcv9-sun-solaris2.11`.
69660cc broke the [Solaris/sparcv9 buildbot](https://lab.llvm.org/staging/#/builders/12/builds/264): `compiler-rt/lib/builtins/int_to_fp.h` unconditionally uses `*int128_t` which don't exist on 32-bit SPARC. As suggested in #67540, this patch fixes this by moving the `CRT_HAS_TF_MODE` guard up which does the necessary checks. Tested on `sparcv9-sun-solaris2.11`.
Builds on #66903, converting the rest of the low-hanging fruit to use the common implementation.
I was holding off posting more of the patch series as it's (sadly) rather awkward on GitHub. But I'll post this next patch in the hope it better demonstrates the direction.