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

tests: math: interpolation: disable floating point contraction #81648

Merged

Conversation

tagunil
Copy link
Collaborator

@tagunil tagunil commented Nov 20, 2024

This test cannot tolerate any loss of precision, including the one caused by the compiler contracting floating points operations together, like in fused multiply-add (FMA). Some toolchains enable FP contraction by default, so disable it for the specific test and let the user decide themselves whether precision or performance is needed for their specific application.

One alternative is to include the pragma into the interpolation function itself and make its behaviour more deterministic. Still, I think I should start with the less intrusive approach.

Another alternative would be to rework the interpolation function to remove the contraction site. For that we basically need to split the FMA expression into two.

@tagunil tagunil requested a review from JordanYates November 20, 2024 10:06
@tagunil tagunil marked this pull request as draft November 20, 2024 13:28
@tagunil tagunil force-pushed the interpolation-no-fp-contract branch from 1feb2b8 to bed8f5c Compare November 20, 2024 14:09
@tagunil tagunil marked this pull request as ready for review November 20, 2024 14:10
@tagunil
Copy link
Collaborator Author

tagunil commented Dec 18, 2024

Hi @JordanYates, could you please take a look and share your thoughts about the proposed solution and its alternatives?

@JordanYates
Copy link
Collaborator

Hi @JordanYates, could you please take a look and share your thoughts about the proposed solution and its alternatives?

I believe I was assigned because I wrote the test, and while I have nothing against the change, this is not an option I have seen before (and it is not present elsewhere in the project). I'm not in a position to know whether options like these are fine by the project, or if we should instead be relaxing the constraints.

I'll try again in discord to get someone with a better idea to review.

@tagunil
Copy link
Collaborator Author

tagunil commented Dec 18, 2024

Hi @JordanYates, could you please take a look and share your thoughts about the proposed solution and its alternatives?

I believe I was assigned because I wrote the test, and while I have nothing against the change, this is not an option I have seen before (and it is not present elsewhere in the project). I'm not in a position to know whether options like these are fine by the project, or if we should instead be relaxing the constraints.

I'll try again in discord to get someone with a better idea to review.

Well, you wrote not just the test, but also the interpolation function itself, so at least you're in a position to know what its design requirements are, and what exactly should be tested. But yes, maybe we need a wider discussion about the solution.

@JordanYates
Copy link
Collaborator

Taking a closer look at this, I find it hard to believe that any floating point operations are so inaccurate that round results in the wrong value. Is there one particular output that falls exactly on a 0.5f multiple that causes the rounding to go different ways between round and roundf? That doesn't seem like the reason since all the multiple are "even" fractions (0.4, 1.6, 1.4, 0.8).

Can you share reproduction steps for the test failure?

@tagunil
Copy link
Collaborator Author

tagunil commented Jan 7, 2025

Taking a closer look at this, I find it hard to believe that any floating point operations are so inaccurate that round results in the wrong value. Is there one particular output that falls exactly on a 0.5f multiple that causes the rounding to go different ways between round and roundf? That doesn't seem like the reason since all the multiple are "even" fractions (0.4, 1.6, 1.4, 0.8).

Can you share reproduction steps for the test failure?

It's the difference between contracted and non-contracted ways of calculating the values subjected to rounding. The y_axis[idx_low] + (slope * x_shifted) expression gives a slightly different output when contracted into a FMA, and there are inputs that make it rounded to the wrong side, because the correct result is a multiple of 0.5f, and the contracted one is a bit lower. I don't have the exact values at hand, but I could produce them later.

The reproduction is as easy as running the test, but, unfortunately, it involves our Clang-based compiler for ARC. Although I think it should be reproducible with any other flavour of Clang, because this compiler behavior is not our addition, but a Clang default.

@JordanYates
Copy link
Collaborator

because the correct result is a multiple of 0.5f, and the contracted one is a bit lower

That was the point I was making with the "even" fractions, none of them should result in a multiple near 0.5f as far as I can tell.

The reproduction is as easy as running the test, but, unfortunately, it involves our Clang-based compiler for ARC. Although I think it should be reproducible with any other flavour of Clang, because this compiler behavior is not our addition, but a Clang default.

I have tried running the test under my native clang and it passes without problems:

~/code/zephyr/zephyr$ export ZEPHYR_TOOLCHAIN_VARIANT=llvm
~/code/zephyr/zephyr$ west build -b native_sim -t run tests/lib/math/interpolation/ -t run
...
-- The C compiler identification is Clang 14.0.0
-- The CXX compiler identification is Clang 14.0.0
-- The ASM compiler identification is Clang with GNU-like command-line
-- Found assembler: /usr/bin/clang
-- Using ccache: /usr/bin/ccache
...
------ TESTSUITE SUMMARY START ------

SUITE PASS - 100.00% [interpolation]: pass = 8, fail = 0, skip = 0, total = 8 duration = 0.000 seconds
 - PASS - [interpolation.test_interpolation_negative_x] duration = 0.000 seconds
 - PASS - [interpolation.test_interpolation_negative_xy] duration = 0.000 seconds
 - PASS - [interpolation.test_interpolation_negative_y] duration = 0.000 seconds
 - PASS - [interpolation.test_interpolation_on_boundary] duration = 0.000 seconds
 - PASS - [interpolation.test_interpolation_oob] duration = 0.000 seconds
 - PASS - [interpolation.test_interpolation_piecewise] duration = 0.000 seconds
 - PASS - [interpolation.test_interpolation_rounding] duration = 0.000 seconds
 - PASS - [interpolation.test_interpolation_simple] duration = 0.000 seconds

------ TESTSUITE SUMMARY END ------

@tagunil
Copy link
Collaborator Author

tagunil commented Jan 7, 2025

OK, that's what I have:

(.venv) tagunil@SNPS-nSrW1MOiLI:~/tests/zephyr$ export ZEPHYR_TOOLCHAIN_VARIANT=arcmwdt
(.venv) tagunil@SNPS-nSrW1MOiLI:~/tests/zephyr$ west build -b nsim/nsim_em -t run tests/lib/math/interpolation/ -t run
...
-- The C compiler identification is Clang 17.0.7
-- The CXX compiler identification is Clang 17.0.7
-- The ASM compiler identification is unknown
...
Running TESTSUITE interpolation
===================================================================
START - test_interpolation_negative_x
 PASS - test_interpolation_negative_x in 0.355 seconds
===================================================================
START - test_interpolation_negative_xy

    Assertion failed at WEST_TOPDIR/zephyr/tests/lib/math/interpolation/src/main.c:127: interpolation_test_interpolation_negative_xy: expected not equal to linear_interpolate(x_axis, y_axis, len, i)

 FAIL - test_interpolation_negative_xy in 0.359 seconds
===================================================================
START - test_interpolation_negative_y

    Assertion failed at WEST_TOPDIR/zephyr/tests/lib/math/interpolation/src/main.c:93: interpolation_test_interpolation_negative_y: expected not equal to linear_interpolate(x_axis, y_axis, len, i)

 FAIL - test_interpolation_negative_y in 0.361 seconds
...

@JordanYates
Copy link
Collaborator

Can you add some manual prints in there to output the loop index, the value of expected before round, and the value of the interpolation function before roundf

@tagunil
Copy link
Collaborator Author

tagunil commented Jan 7, 2025

I've found a previously recorded set of failing numbers: y_axis[idx_low] equals to -10, slope equals to 0.100000001 (exact value is 0x3dcccccd), and x_shifted is 85. The non-contracted expression gives -1.5 (aka 0xbfc00000), and the contracted one gives -1.49999988 (aka 0xbfbfffff).

@tagunil
Copy link
Collaborator Author

tagunil commented Jan 7, 2025

Can you add some manual prints in there to output the loop index, the value of expected before round, and the value of the interpolation function before roundf

Sure, will do in half a hour.

Copy link
Collaborator

@JordanYates JordanYates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with the solution, as long as a comment is added as a reference for why it is there (see suggestion).

tests/lib/math/interpolation/src/main.c Show resolved Hide resolved
@tagunil
Copy link
Collaborator Author

tagunil commented Jan 7, 2025

Can you add some manual prints in there to output the loop index, the value of expected before round, and the value of the interpolation function before roundf

For negative_y:

i = 2985
expected = -1.50000000
actual = -1.49999988

For negative_xy:

i = -2015
expected = -1.50000000
actual = -1.49999988

This test cannot tolerate any loss of precision, including the one
caused by the compiler contracting floating points operations together,
like in fused multiply-add (FMA). Some toolchains enable FP contraction
by default, so disable it for the specific test and let the user decide
themselves whether precision or performance is needed for their
specific application.

Co-authored-by: Jordan Yates <[email protected]>
Co-authored-by: Ilya Tagunov <[email protected]>
Signed-off-by: Ilya Tagunov <[email protected]>
@tagunil tagunil force-pushed the interpolation-no-fp-contract branch from bed8f5c to e9e058b Compare January 8, 2025 14:19
@tagunil tagunil requested a review from JordanYates January 8, 2025 17:34
@kartben kartben merged commit 48d2382 into zephyrproject-rtos:main Jan 9, 2025
18 checks passed
@tagunil tagunil deleted the interpolation-no-fp-contract branch January 9, 2025 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants