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

Remove unnecessary volatiles from volk_32fc_s32f_magnitude_16i #724

Merged
merged 1 commit into from
Dec 17, 2023

Conversation

argilo
Copy link
Member

@argilo argilo commented Dec 14, 2023

Several variables in volk_32fc_s32f_magnitude_16i were marked as volatile in an attempt to resolve a flaky test:

#282 (comment)
#282 (comment)
#285

These changes did not actually resolve the flaky test; it still failed on i386, and the test itself was later fixed in #713. Therefore, we can now revert the additions of volatile that were made in #285.

@argilo
Copy link
Member Author

argilo commented Dec 14, 2023

Removing the volatiles speeds up the generic protokernel significantly. The sse protokernel also sees a small improvement.

Before:

RUN_VOLK_TESTS: volk_32fc_s32f_magnitude_16i(131071,1987)
generic completed in 757.907 ms
a_avx2 completed in 75.477 ms
a_sse3 completed in 432.871 ms
a_sse completed in 466.542 ms
u_avx2 completed in 76.9684 ms

After:

RUN_VOLK_TESTS: volk_32fc_s32f_magnitude_16i(131071,1987)
generic completed in 471.106 ms
a_avx2 completed in 77.2529 ms
a_sse3 completed in 417.156 ms
a_sse completed in 399.88 ms
u_avx2 completed in 76.719 ms

Copy link
Contributor

@jdemel jdemel left a comment

Choose a reason for hiding this comment

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

LGTM

@jdemel jdemel merged commit f653de9 into gnuradio:main Dec 17, 2023
32 checks passed
@argilo argilo mentioned this pull request Dec 17, 2023
Alesha72003 pushed a commit to Alesha72003/volk that referenced this pull request May 15, 2024
Remove unnecessary volatiles from volk_32fc_s32f_magnitude_16i
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.

2 participants