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 broken sse_32 protokernels #714

Merged
merged 1 commit into from
Dec 10, 2023
Merged

Conversation

argilo
Copy link
Member

@argilo argilo commented Dec 8, 2023

volk_32fc_x2_conjugate_dot_prod_32fc fails in i386 Debian CI and locally. Only the a_sse_32 protokernel is broken:

108: offset 0 in1: 184.314 + -210.344j  in2: 183.667 + -210.042j tolerance was: 0.02
108: volk_32fc_x2_conjugate_dot_prod_32fc: fail on arch a_sse_32

I had a look over the code, and it appears the problem is that inline assembler writes to input, taps, and *result without telling the compiler about that. I made a few attempts at fixing it, but I'm not sure whether it's worth saving this ugly code. I doubt this protokernel sees much use, since it is only available on 32-bit systems, and many newer protokernels are now available on 32-bit x86.

It appears volk_32fc_x2_dot_prod_32fc_a_sse_64 had a similar bug, as it was commented out in 2012 and remains commented out today:

gnuradio/gnuradio@43224ac

I've removed that protokernel here as well. After this change, there are no more sse_32 protokernels left in VOLK.

Signed-off-by: Clayton Smith <argilo@gmail.com>
@argilo argilo added the bug label Dec 8, 2023
@argilo
Copy link
Member Author

argilo commented Dec 8, 2023

I checked whether GNU Radio uses the volk_32fc_x2_conjugate_dot_prod_32fc kernel, and it only recently started doing so in gnuradio/gnuradio#3306. That's another good indication that the buggy volk_32fc_x2_conjugate_dot_prod_32fc_a_sse_32 protokernel hasn't seen any use.

@argilo argilo mentioned this pull request Dec 9, 2023
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. Less assembly should improve maintenance as well. I wonder if this kernel is ever used nowadays.

@jdemel jdemel merged commit 523706f into gnuradio:main Dec 10, 2023
33 checks passed
Alesha72003 pushed a commit to Alesha72003/volk that referenced this pull request May 15, 2024
Remove broken sse_32 protokernels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants