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

Add length checks to volk_8u_x2_encodeframepolar_8u #642

Merged
merged 1 commit into from
Oct 22, 2023

Conversation

argilo
Copy link
Member

@argilo argilo commented Oct 12, 2023

Fixes #414.

The various volk_8u_x2_encodeframepolar_8u implementations have warnings about the input size:

// This last part requires at least 16-bit frames.
// Smaller frames are useless for SIMD optimization anyways. Just choose GENERIC!

// This last part requires at least 32-bit frames.
// Smaller frames are useless for SIMD optimization anyways. Just choose GENERIC!

// This last part requires at least 16-bit frames.
// Smaller frames are useless for SIMD optimization anyways. Just choose GENERIC!

// This last part requires at least 32-bit frames.
// Smaller frames are useless for SIMD optimization anyways. Just choose GENERIC!

So let's heed those warnings by falling back to the generic implementation if the input size is too small.

Doing so also removes the need for extra size checks in volk_32f_8u_polarbutterfly_32f.h.

After this change, all failures disappear. I tested with the following:

for i in {1..64}; do apps/volk_profile -n -i 1 -v $i -R encodepolar | grep fail; done

I also verified that all valgrind warnings disappear after these changes. I tested with:

for i in {1..64}; do valgrind apps/volk_profile -n -i 1 -v $i -R encodepolar | grep fail; done

} else {
volk_8u_x2_encodeframepolar_8u_generic(u_target, u_temp, stage_size);
}
volk_8u_x2_encodeframepolar_8u_u_avx2(u_target, u_temp, stage_size);
Copy link
Member Author

Choose a reason for hiding this comment

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

May as well call the AVX2 version here, since we're in volk_32f_8u_polarbutterfly_32f_u_avx2.

@argilo
Copy link
Member Author

argilo commented Oct 12, 2023

I also confirmed that GNU Radio's qa_polar_decoder_sc_systematic_test test passes after this change, even when VOLK_GENERIC=1 is removed:

diff --git a/cmake/Modules/GrTest.cmake b/cmake/Modules/GrTest.cmake
index b5b2c196ec..f31e199067 100644
--- a/cmake/Modules/GrTest.cmake
+++ b/cmake/Modules/GrTest.cmake
@@ -67,7 +67,7 @@ function(GR_ADD_TEST test_name)
     # We add it to the beginning of the list to use locally-built modules before installed ones.
     list(INSERT pypath 0 "${PROJECT_BINARY_DIR}/test_modules")
 
-    set(environs "VOLK_GENERIC=1" "GR_DONT_LOAD_PREFS=1" "srcdir=${srcdir}"
+    set(environs "GR_DONT_LOAD_PREFS=1" "srcdir=${srcdir}"
                  "GR_CONF_CONTROLPORT_ON=False")
     list(APPEND environs ${GR_TEST_ENVIRONS})
 

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. When I implemented these kernels, these very small sizes seemed to be useless for an FEC code. But people stumble over it anyways. Thus, we should fix that.

@argilo
Copy link
Member Author

argilo commented Oct 14, 2023

If GNU Radio's qa_polar_decoder_sc_systematic_test test uses an unrealistic code size, maybe it should be fixed.

@argilo
Copy link
Member Author

argilo commented Oct 14, 2023

Another motivation for making this change is that it would allow a CI test to sweep over small vector lengths (which are prone to bugs) in all kernels.

@jdemel jdemel merged commit da574b4 into gnuradio:main Oct 22, 2023
@argilo argilo deleted the fix-polar branch December 2, 2023 16:50
Alesha72003 pushed a commit to Alesha72003/volk that referenced this pull request May 15, 2024
Add length checks to volk_8u_x2_encodeframepolar_8u
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.

Problem with POLAR encoding/decoding blocks
2 participants