Skip to content
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

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

asb
Copy link
Contributor

@asb asb commented Sep 27, 2023

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.

Builds in llvm#66903, converting the rest of the low-hanging fruit to use
the common implementation.
@asb asb force-pushed the 2023q3-compiler-rt-refactor-int-to-fp-2 branch from 30951ff to 475c6cb Compare October 17, 2023 14:02
@asb
Copy link
Contributor Author

asb commented Oct 17, 2023

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 __clzti2 is called (there is no __builtin_clz variant in Clang for i128, so no way to generate llvm.ctlz.i128 calls). It doesn't understand it's semantics and seemingly the conservative analysis it does results in slightly different cases where poison is introduced for the two functions. However, massaging the output to replace that call with one to llvm.ctlz.i128 does show equivalence. I did encounter a gotcha whereby incorrectly doing this replacement (e.g. getting the signature wrong) would indicate llvm.ctlz.i128 = constant ? bytes, align 8 in Alive's output and it confidently claimed equivalence when e.g. changing one of the call args to a constant. The linked comparisons avoid this issue though.

@alexander-shaposhnikov
Copy link
Collaborator

The changes look good to me, thank you.
One thing that I did for testing (that gave me a bit more confidence) was verifying that the conversions produce exactly the same results as before for all possible values of small types (up to float). E.g. one can save all pairs (src_value, dst_value) into old.txt and new.txt and compare them.

@asb asb merged commit 69660cc into llvm:main Oct 18, 2023
2 checks passed
@arichardson
Copy link
Member

This seems to have broken __floatuntitf for x86_64 (noticed after rebasing #68132 on top of this).

@arichardson
Copy link
Member

__floattitf(1) is returning 0.0 instead of 1.0 with this change.

@arichardson
Copy link
Member

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.

@rorth
Copy link
Collaborator

rorth commented Oct 23, 2023

This patch badly broke the Solaris/sparcv9 buildbot which has 128-bit long double, but no *int128_t.

@@ -16,6 +16,10 @@
#include "fp_lib.h"
#include "int_lib.h"

#define SRC_U128
Copy link
Member

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.

Copy link
Member

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

Copy link
Collaborator

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.hisn't more robust, unconditionally using types whose existance isn't guaranteed everywhere.

rorth added a commit to rorth/llvm-project that referenced this pull request Oct 24, 2023
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`.
rorth added a commit that referenced this pull request Oct 24, 2023
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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants