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

Generate random integers with uniform_int_distribution #677

Merged
merged 2 commits into from
Nov 4, 2023

Conversation

argilo
Copy link
Member

@argilo argilo commented Oct 23, 2023

Fixes #688.

Random integer generation in load_random_data has a lot of bugs:

  • float(uint64_t(2) << (type.size * 8)) performs a left shift by 64 bits when the type size is 8, and this results in Undefined Behaviour. On my system, the only generated uint64_t values are 0, 1, and 18446744073709551615.
  • The float(uint64_t(2) << (type.size * 8)) calculation is also incorrect (twice as large as it should be) when the type size is smaller than 8, resulting in Undefined Behaviour when floating-point random values are cast to integers.
  • Floating-point values have fewer bits of precision than 32-bit integers, so most generated int32_t and uint32_t values are multiples of 256.
  • The only generated uint16_t values are 0, 1, 2, 3, 4, 5, 6, 7, 65529, 65530, 65531, 65532, 65533, 65534, and 65535.

Here I've fixed all these problems by replacing std::uniform_real_distribution with std::uniform_int_distribution.

I had to keep int16_t generation in the range -7 .. +7, because various kernels added in #77 fail otherwise (due to differing integer overflow between implementations).

In a separate commit, I also fixed the compiler warnings in qa_utils.cc:

@argilo
Copy link
Member Author

argilo commented Oct 23, 2023

This has revealed that volk_64u_byteswap_neonv8 is broken. I guess we'll have to fix that first.

@marcusmueller
Copy link
Member

This has revealed that volk_64u_byteswap_neonv8 is broken. I guess we'll have to fix that first.

I'll argue that falling back to the generic implementation is OK here and we should not prioritize fixing it (instead of removing it) over fixing all our test cases

@marcusmueller
Copy link
Member

From a build on aarch64 g++12 Ubuntu22.04:

RUN_VOLK_TESTS: volk_64u_byteswappuppet_64u(131071,1)
generic_decompose completed in 0.947894 ms
generic completed in 0.596396 ms
neonv8 completed in 4.90347 ms
Best aligned arch: generic
Best unaligned arch: generic
Test #125: qa_volk_64u_byteswappuppet_64u .......................   Passed    0.17 sec

(generic_decompose is the current generic, generic is my new proposed one.)

Same picture on the older g++8 of Ubuntu 20.04.

In other words, yeah, let's kill that broken neonv8 kernel. It serves no purpose that generic (even the old one) doesn't do better, in all our current builds.

@argilo
Copy link
Member Author

argilo commented Oct 23, 2023

It looks like the neonv8 kernel was even slower back when it was added: #196

@argilo
Copy link
Member Author

argilo commented Oct 23, 2023

I'll rebase this after #680 is merged.

@argilo argilo mentioned this pull request Oct 31, 2023
@jdemel jdemel linked an issue Nov 4, 2023 that may be closed by this pull request
@argilo
Copy link
Member Author

argilo commented Nov 4, 2023

I rebased on main (to pick up #680) and the tests now pass.

@jdemel jdemel merged commit d5b317c into gnuradio:main Nov 4, 2023
33 checks passed
@argilo argilo deleted the qa-utils-fixes branch December 2, 2023 16:48
Alesha72003 pushed a commit to Alesha72003/volk that referenced this pull request May 15, 2024
Generate random integers with uniform_int_distribution
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.

Poor random numbers in uint tests.
3 participants