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

Simplify the Spiral-generated convolutional decoder #756

Merged
merged 2 commits into from
Feb 10, 2024

Conversation

argilo
Copy link
Member

@argilo argilo commented Feb 1, 2024

Here I've continued cleaning up the convolutional decoder.

Since #755 is not merged yet, I've included it here, and will rebase the PR once it's merged.

In two additional commits I've made the following changes:

  1. Remove loop unrolling. Each of the SIMD protokernels had an unrolled loop that processed two bits of input in a row, as well as special processing for the leftover bit, if any. Since the processing step for each bit is large, I don't see any point in unrolling the loop, and removing the unrolling does not appear to have any impact on performance.
  2. Remove temporary variables. Spiral generates temporary variables for every single quantity and memory address, which makes the code difficult to understand. I removed these, which again appears to have no impact on performance but makes the code much more readable.

With these changes, the tests still pass. I also verified (on x86 and ARM) that various vector lengths still work:

for v in {0..1024}; do apps/volk_profile -n -R k7_r2 -v $v -i 10 | grep fail; done

Signed-off-by: Clayton Smith <[email protected]>
Signed-off-by: Clayton Smith <[email protected]>
@argilo
Copy link
Member Author

argilo commented Feb 9, 2024

Rebased.

@jdemel
Copy link
Contributor

jdemel commented Feb 10, 2024

@argilo Thanks! I really appreciate your work here. Before I merge this, how are the test run? We had a lot of issues with this kernel in past. I'd like to make sure, we don't screw this kernel up again.

@argilo
Copy link
Member Author

argilo commented Feb 10, 2024

CI should do a decent job of testing the kernel now, since it verifies that all the protokernels produce identical bit streams when given noise as input. One thing that CI doesn't yet do is test with different vector lengths, which I did like so:

for v in {0..1024}; do apps/volk_profile -n -R k7_r2 -v $v -i 10 | grep fail; done

@argilo
Copy link
Member Author

argilo commented Feb 10, 2024

I ran that test on x86-64, ARM-64, and ARM-32.

@jdemel jdemel merged commit aac4c7f into gnuradio:main Feb 10, 2024
34 checks passed
@jdemel
Copy link
Contributor

jdemel commented Feb 10, 2024

Great. Merging.

@argilo argilo deleted the simplify-conv branch February 10, 2024 18:11
Alesha72003 pushed a commit to Alesha72003/volk that referenced this pull request May 15, 2024
Simplify the Spiral-generated convolutional decoder
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