This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add a str() method to FPBits which returns a string representation.
ClosedPublic

Authored by sivachandra on May 18 2023, 1:56 PM.

Details

Summary

Unit tests for the str() method have also been added.

Previously, a separate test only helper function was being used by the
test matchers which has regressed over many cleanups. Moreover, being a
test only utility, it was not tested separately (and hence the
regression).

Diff Detail

Event Timeline

sivachandra created this revision.May 18 2023, 1:56 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 18 2023, 1:56 PM
sivachandra requested review of this revision.May 18 2023, 1:56 PM
sivachandra updated this revision to Diff 523547.

clang-format

I have not yet verified that the non-x86 long double values are correct.

michaelrj added inline comments.May 18 2023, 2:12 PM
libc/test/src/__support/FPUtil/fpbits_test.cpp
146

there are three types of long double we support, 80 bit, 128 bit, and 64 bit. These tests won't work on 64 bit long double systems.

sivachandra added inline comments.May 18 2023, 2:16 PM
libc/test/src/__support/FPUtil/fpbits_test.cpp
146

We skip doing long double checks completely if long double == double on line 125.

I'll test this patch on aarch64 to check the float128 support

libc/test/src/__support/FPUtil/fpbits_test.cpp
146

ah, that makes sense.

Fix long double tests on aarch64.

This revision is now accepted and ready to land.May 18 2023, 2:45 PM
lntue added inline comments.May 18 2023, 3:03 PM
libc/test/src/__support/FPUtil/fpbits_test.cpp
124

How about having 2 separate long double tests X86LongDoubleType and QuadPrecisionLongDoubleType? You can have them return early by testing:

if constexpr( fputil::FloatProperties<long double>::MANTISSA_WIDTH != ... )
  return;

Separate out x86 long double and other long double tests.

sivachandra added inline comments.May 18 2023, 11:20 PM
libc/test/src/__support/FPUtil/fpbits_test.cpp
124

I have not done that exactly but separated out the x86 long double tests into a different test enabled only for x86. The reason I did not go with your approach is that, it would be weird to see QuadPrecisionLongDoubleType test passing on x86 (and also the other way round on non-x86).

This revision was landed with ongoing or failed builds.May 18 2023, 11:21 PM
This revision was automatically updated to reflect the committed changes.