csqrt_test on platforms with 128-bit long double
ClosedPublic
Actions

Authored by arichardson on Mar 5 2021, 11:17 AM.

Details

Reviewers

rlibby
markj
ngie
lwhsu

Commits

rG47950e427c5f: Fix lib/msun/tests/csqrt_test on platforms with 128-bit long double
rGce88eb476b86: Fix lib/msun/tests/csqrt_test on platforms with 128-bit long double

Summary

If long double has more than 64 mantissa bits, using uint64_t to hold the
mantissa bits will truncate the value and result in test failures. To fix
this problem use uint128_t since all platforms that have
LDBL_MANT_DIG__ > 64 also have compiler support for 128-bit integers.

Depends on D28798

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 37825
Build 34714: arc lint + arc unit

Event Timeline

arichardson created this revision.Mar 5 2021, 11:17 AM

Herald added a subscriber: imp. · View Herald TranscriptMar 5 2021, 11:17 AM

arichardson requested review of this revision.Mar 5 2021, 11:17 AM

Harbormaster completed remote builds in B37594: Diff 85188.Mar 5 2021, 11:17 AM

rlibby added inline comments.Mar 5 2021, 4:05 PM

lib/msun/tests/csqrt_test.c
293–297	What is CHECK_FPEQUAL? I don't see it by grepping or in the review stack.
348–354	I don't get either the FIXME comment or how this fixes i386. Can you explain?

arichardson added inline comments.Mar 5 2021, 4:10 PM

lib/msun/tests/csqrt_test.c
293–297	Sorry, this is missing and added by another review that I apparently haven't uploaded yet. It's just a wrapper around ATF_CHECK that also prints the mismatched values.
348–354	I added the FIXME in an earlier commit since using DBL_MANT_DIG for AArch64 seemed wrong. LDBL_MANT_DIG would previously result in overflow so was wrong for all architectures with LDBL_MANT_DIG > 64. I haven't tested i386, but since it should be using ld80 I would expect it to behave in the same way as amd64?

rlibby added inline comments.Mar 5 2021, 4:34 PM

lib/msun/tests/csqrt_test.c
293–297	Okay, that would be fine. I just wanted to make sure it was still an exact check.
348–354	I think you might be reading it backward? Only i386 gets DBL_MANT_DIG, because i386 long doubles are weird and broken. It was probably confusing and bad style to use `ifndef` instead of `ifdef`.

arichardson added inline comments.Mar 5 2021, 4:36 PM

lib/msun/tests/csqrt_test.c
348–354	Ah indeed I did invert the condition. Will update revision to drop this change.

arichardson added a parent revision: D29091: Improve test messages for msun tests.Mar 5 2021, 4:36 PM

arichardson added inline comments.

lib/msun/tests/csqrt_test.c
293–297	I've now uploaded it as D29091.

arichardson mentioned this in D29092: lib/msun/tests: Re-enable csqrt_test on AArch64.Mar 5 2021, 4:40 PM

arichardson added a child revision: D29092: lib/msun/tests: Re-enable csqrt_test on AArch64.Mar 5 2021, 4:40 PM

Change looks fine once the i386 hack is restored.

lib/msun/tests/csqrt_test.c
260–272	Maybe like this? This is just for testing the test though, not the interesting code, so take it or leave it.

This revision now requires changes to proceed.Mar 5 2021, 11:10 PM

Apply review suggestions and add a comment regarding the i386 ifdef

Harbormaster completed remote builds in B37825: Diff 85723.Mar 14 2021, 2:48 PM

rlibby added inline comments.Mar 14 2021, 8:25 PM

lib/msun/tests/csqrt_test.c
262	128
355	It's weirder than this... Here's a reference: https://lists.freebsd.org/pipermail/freebsd-numerics/2012-September/000288.html In short, I would change "precision" to "rounding" or "rounding precision", otherwise the comment doesn't seem to explain why LDBL_MANT_DIG isn't just 53 on i386.