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

Make unit tests more robust across different architectures #442

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

roehling
Copy link
Contributor

This PR deals with a few architectural idiosyncrasies that came up in Debian:

  • The index lookups are sensitive to endianness
  • Floating point equality comparisons are generally considered flaky, which is why GTest provides EXPECT_FLOAT_EQ. The i387 coprocessor with its internal 80 bit extended precision makes things especially awkward.

@solidpixel
Copy link
Contributor

solidpixel commented Nov 28, 2023

The index lookups are sensitive to endianness

Ack, seems sensible to fix. Just want to make sure this is a portable way to fix the problem (we get some esoteric compilers and old compiler versions used in embedded systems).

Floating point equality comparisons are generally considered flaky, which is why GTest provides EXPECT_FLOAT_EQ.

The main point of these tests is to ensure that the SIMD is actually bit-exact with the non-SIMD code on a specific CPU, rather than prove out any kind of floating point cross architecture, so in this case the exact comparison was somewhat deliberate. These tests work totally fine on both Arm64 and x86-64 as both are defined to have the same floating point behavior for both SIMD and scalar ops.

Is there actually a real use case for supporting i387 beyond it being an interesting piece of architectural history? We've long since dropped 32-bit platforms from our testing, and I have no way (or desire) to add any testing to ensure it doesn't break again in future, so I'm a little reluctant to add code to handle it.

@solidpixel solidpixel self-assigned this Nov 28, 2023
@roehling
Copy link
Contributor Author

The main point of these tests is to ensure that the SIMD is actually bit-exact with the non-SIMD code on a specific CPU, rather than prove out any kind of floating point cross architecture, so in this case the exact comparison was somewhat deliberate.

Ah, I was not aware of that, but I see the point. And it does seem to work on most architectures as intended.

Is there actually a real use case for supporting i387 beyond it being an interesting piece of architectural history? We've long since dropped 32-bit platforms from our testing.

I would assume about as many use-cases as for esoteric/old compilers on embedded systems ;)
Joking aside, probably not. The i386 architecture is mostly relevant for running legacy binaries such as Steam games these days.

Do you want me to revert the second commit from this PR, or will you do it yourself?

@solidpixel
Copy link
Contributor

I can do that, no probs.

@solidpixel
Copy link
Contributor

FYI - fixing the tests to work for BE is trivial, but fixing the codec to support BE properly is likely a bit more work. Happy to do it, but might take a few days as I need to get a BE test environment set up.

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.

None yet

2 participants