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

fix test_mm_dp_pd test #643

Merged
merged 5 commits into from
Jul 25, 2024
Merged

Conversation

alexorlov124
Copy link
Contributor

When I ran make check with -O2 passed to ARCH_CFLAGS, the test_mm_dp_pd failed. After debugging, I discovered that a buffer initialized with floats was being cast to a buffer of doubles. The root cause of the test failure was that the exact match of the dot product obtained with intrinsics and calculated with C++ doubles showed differences. This occurred because the numbers were too large, and the dot product had rounding errors.

@howjmay
Copy link
Collaborator

howjmay commented Jul 18, 2024

I think you use the different version of clang-format. It should be clang-format-12. But you can refer CI directly to format the code

@alexorlov124
Copy link
Contributor Author

I think you use the different version of clang-format. It should be clang-format-12. But you can refer CI directly to format the code

Thank you, I installed clang-formatter 12 and now format is fixed

@howjmay
Copy link
Collaborator

howjmay commented Jul 20, 2024

Btw may I ask which compiler and platform you used to trigger the error?

@alexorlov124
Copy link
Contributor Author

alexorlov124 commented Jul 20, 2024

Btw may I ask which compiler and platform you used to trigger the error?

I used gcc 10.2 and 11.4, Graviton3 and Amazon Linux 2023

@howjmay
Copy link
Collaborator

howjmay commented Jul 20, 2024

Sorry it seems not to pass the CI. May you help me to enable -O2 in the CI and check it ?

https://github.com/howjmay/sse2neon/actions/runs/10019289097

@alexorlov124
Copy link
Contributor Author

Sorry it seems not to pass the CI. May you help me to enable -O2 in the CI and check it ?

https://github.com/howjmay/sse2neon/actions/runs/10019289097

I updated .github\workflows\main.yml with -O2 flag not sure we should merge it in master but we can make sure tests are green

@alexorlov124
Copy link
Contributor Author

Here my pull request with optimization enabled #644

Copy link
Member

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Squash the git commits and refine the commit messages.
See https://cbea.ms/git-commit/

@howjmay
Copy link
Collaborator

howjmay commented Jul 20, 2024

Here my pull request with optimization enabled #644

Thank you for the test. However, it seems the test didn't pass. Not sure if I did something wrong.

I think this PR should be ok to merge after squash. But I just feel strange. It seems this PR can't solve the error when O2 is given

@jserv jserv merged commit 1a854ed into DLTcollab:master Jul 25, 2024
16 checks passed
@jserv
Copy link
Member

jserv commented Jul 25, 2024

Thank @alexorlov124 for contributing!

@howjmay
Copy link
Collaborator

howjmay commented Jul 25, 2024

I will investigate what causes O2 fails in #642, since this PR doesnt solve the problem in O2

@alexorlov124
Copy link
Contributor Author

alexorlov124 commented Jul 25, 2024 via email

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