This is an archive of the discontinued LLVM Phabricator instance.

[libc] Resolve NaN/implementation-defined behavior of floating-point tests
ClosedPublic

Authored by ddcc on Sep 29 2022, 3:33 PM.

Diff Detail

Event Timeline

ddcc created this revision.Sep 29 2022, 3:33 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 29 2022, 3:33 PM
ddcc requested review of this revision.Sep 29 2022, 3:33 PM
ddcc updated this revision to Diff 464071.

Remove deprecated change

michaelrj added inline comments.
libc/src/__support/FPUtil/FPBits.h
95

if you're going to make these UIntType then you should change the variable to be UIntType as well, but that shouldn't be necessary as the largest float we need to handle is quadruple precision which uses 15 bits for its exponent, and int is defined as being at least 16 bits.

169

this would make it impossible to generate a signaling NaN with this function, since it would always set the quiet bit. Currently the expectation is that the client code will pass a mantissa with the quiet bit set iff the resulting NaN should be quiet.

libc/test/src/math/RoundToIntegerTest.h
87

is this flag set anywhere?

lntue added inline comments.Sep 30 2022, 2:46 PM
libc/src/__support/FPUtil/FPBits.h
169

It's probably better to add another method called build_quiet_nan and leave build_nan to create signaling nan if needed? Most of build_nan usage in src/math folder then can simply be replaced with build_quiet_nan.

ddcc updated this revision to Diff 464854.Oct 3 2022, 5:00 PM
ddcc marked 3 inline comments as done.

Use build_quiet_nan and update CMakeLists.txt

ddcc marked an inline comment as done.Oct 3 2022, 5:01 PM
lntue added inline comments.Oct 3 2022, 9:34 PM
libc/src/__support/FPUtil/FPBits.h
174

I think you meant to call build_nan. Otherwise this is an infinite recursion.

ddcc updated this revision to Diff 465203.Oct 4 2022, 3:23 PM

Fix infinite recursion from sed replace

lntue accepted this revision.Oct 4 2022, 3:59 PM
This revision is now accepted and ready to land.Oct 4 2022, 3:59 PM
ddcc updated this revision to Diff 465553.Oct 5 2022, 2:16 PM

Fix x86_64 long double

lntue accepted this revision.Oct 5 2022, 3:28 PM
michaelrj added inline comments.Oct 5 2022, 4:43 PM
libc/utils/UnitTest/FPMatcher.h
75

the build failures were caused by this line. Changing it back to build_nan fixes the issue. Maybe add qNaN as an additional variable for quiet NaN purposes.

lntue added inline comments.Oct 5 2022, 5:50 PM
libc/utils/UnitTest/FPMatcher.h
75

Look like the failures are actually in the implementation and tests of atanf and atanhf. According to the spec, we should not set floating point exceptional flags when the input is NaN, signaling or quiet. You can change the atanf_test and atanhf_test to EXPECT_FP_EXCEPTION(0); for aNaN inputs and simply return x in atanf.cpp and atanhf.cpp for NaN inputs instead of x + 1.0f as of now.

ddcc reopened this revision.Oct 6 2022, 3:34 PM
This revision is now accepted and ready to land.Oct 6 2022, 3:34 PM
ddcc updated this revision to Diff 465908.Oct 6 2022, 3:34 PM

Fix atan behavior

ddcc marked 2 inline comments as done.Oct 6 2022, 3:36 PM
ddcc added inline comments.
libc/utils/UnitTest/FPMatcher.h
75

Thanks for the help! Is there an easy way to rerun the test? ninja check-llvmlibc only works once.

lntue added inline comments.Oct 6 2022, 5:52 PM
libc/utils/UnitTest/FPMatcher.h
75

Normally I would simply $ touch <test_files> or $ touch <source_files>, and then simply run the test directly with $ ninja libc.test.src.math.<test>, unless the changes are in CMakeFiles.txt

This revision was automatically updated to reflect the committed changes.
ddcc marked an inline comment as done.