Use pointers to pass in s32fc arguments

[argilo]

Fixes #442.
Will allow gnuradio/gnuradio#6300 and gqrx-sdr/gqrx#781 to be fixed.

VOLK uses the same function signatures in both C and C++. This mostly works, except for arguments of type lv_32fc_t, which are float complex in C and std::complex<float> in C++. The language specifications for C and C++ both guarantee that these have the same memory representation, but not that they have the same calling convention.

To avoid the problem, we can replace the three instances of lv_32fc_t arguments with lv_32fc_t*, which should work in both C and C++. This change intentionally breaks the API, since the current API has Undefined Behaviour. If merged, the next version of VOLK should be 4.0.0 to reflect the API incompatibility. To preserve backwards compatibility, I've made the current kernels into wrappers around new ones. Users wishing to use the new kernels while still supporting older VOLK versions could do the following:

    void rotateN(gr_complex* out, const gr_complex* in, int n)
    {
#if VOLK_VERSION >= 030100
        volk_32fc_s32fc_x2_rotator2_32fc(out, in, &d_phase_incr, &d_phase, n);
#else
        volk_32fc_s32fc_x2_rotator_32fc(out, in, d_phase_incr, &d_phase, n);
#endif
    }

After these changes, VOLK's tests pass on all platforms, including ppc64le and s390x.

Affected kernels are:

• volk_32fc_s32fc_multiply_32fc (12 usages in GNU Radio)
• volk_32fc_s32fc_x2_rotator_32fc (1 usage in GNU Radio)
• volk_32fc_x2_s32fc_multiply_conjugate_add_32fc (not used in GNU Radio)

[argilo]

I made this change because list_cpu_features does not seem to be present on s390x, which caused CI to fail.

[marcusmueller]

It is worth noting that in the x86_64 SYS V C ABI, complex T arguments are passed through memory, anyways (p.22, "Passing", clause 5), so the pointer deref happens anyways. We're not introducing any extra overhead for the "flagship" architecture here.

[argilo]

I forgot to update the code sample for volk_32fc_s32fc_x2_rotator_32fc, so I pushed and update to include that. Also, I updated the documentation to clarify that phase_inc and phase are scalars.

The documentation for volk_32fc_s32fc_multiply_32fc and volk_32fc_x2_s32fc_multiply_conjugate_add_32fc already makes it clear that the affected inputs are scalars.

[argilo]

Note: This PR is an alternative to #488, which proposes to replace VOLK's C++ API, but is stalled due to difficulties with MSVC. In comparison, this PR makes a less drastic API change, but does have the drawback of using pointers to pass in complex scalar inputs, which could be confusing. (Elsewhere in VOLK, pointers are only used for vectors and scalar outputs.)

I think it's worth accepting that drawback to have VOLK (and packages that depend on it, like GNU Radio and Gqrx) run on all architectures.

[jdemel]

I'd like to summarize our problem here too.

From its inception, VOLK is a C library with an interface that pretends to be C++ in some cases. This is an issue because C complex and std::complex are not necessarily binary compatible as discussed in #442 .

The clean solution here would be:

• implement VOLK in C and C only
• add a wrapper that presents a C++ API.

This sounds like a simple solution but it isn't. Besides multiple hardware architectures (x86, ARM, ...) we also target multiple platforms (Linux, Mac, Windows). On Windows, the default compiler is MSVC. The MSVC C compiler lacks C complex support.
In order to make things work, VOLK received this initial interface that essentially relies on undefined behavior. The reason for this approach was probably to make it work with MSVC because we change all the complex definitions to look like std::complex for MSVC and compile VOLK as a C++ library in this case.

I thought about the proposed solution here. However, this solution makes the interface less verbose. The whole "pass a single value by pointer" thing works but obstructs the interface. The VOLK interface itself should be as verbose as possible. If a scalar is not passed by value, it is easier to misinterpret this as a vector input. We need additional documentation that is easy to miss in this case.

Some alternative approaches might be:

• Transform VOLK into a C++ library that provides a C interface
• Figure out if we can compile VOLK with clang on Windows. Go for the "clean approach" and figure out how we make things work with MSVC as a shared library consumer.

[argilo]

Thanks for providing a summary of the issue.

I agree that API changes should be carefully considered, and I'd be happy to see one of the alternatives adopted in place of what I've proposed here. This PR is intended as a pragmatic solution in case the alternatives don't work out.

Until a solution is found, users could work around this issue by calling the affected kernels from C. I might take a crack at making that change in GNU Radio.

[jdemel]

Would it be possible to add wrappers around the new, pointer based, API that replicates the currrent, value based, API? In this case, we could mark the value based API as deprecated and issue a warning. We do this with other kernels as well.
The big benefit would be that we don't have to do another major release. e.g. GNU Radio would have time to adopt. This kind of change is difficult to ingest shortly before the Ubuntu 24.04 deadline.

[argilo]

That sounds like a reasonable approach. I'll take a crack at it to see how it goes.

[argilo]

users could work around this issue by calling the affected kernels from C

I tried this, but ran into the difficulty that MSVC doesn't support (standard) complex numbers in C, which precludes using VOLK from C. I believe this is same problem that stalled #488.

[argilo]

Would it be possible to add wrappers around the new, pointer based, API that replicates the currrent, value based, API? In this case, we could mark the value based API as deprecated and issue a warning.

I've updated the PR to take this approach, and CI seems to be happy.

I'll update the documentation for the old kernels as well to point to the new ones.

[argilo]

I tested this against GNU Radio (main branch). GNU Radio compiles (with deprecation warnings) and tests still pass.

Then I updated VOLK's version number to 3.0.1, and applied the following patch to GNU Radio: gnuradio/gnuradio@main...argilo:gnuradio:volk-s32fc-pointer

After this change, the deprecation warnings disappear and all tests still pass.

[argilo]

In three cases, GNU Radio was using volk_32fc_s32fc_multiply_32fc with a scalar that was in fact a float, so I switched those over to volk_32f_s32f_multiply_32f instead. (Presumably this will be more efficient too.)

[argilo]

The updated GNU Radio code also passes CI with VOLK 3.0.0, so it should be possible to use any combination of old/new GNU Radio with old/new VOLK.

[argilo]

I tested the VOLK & GNU Radio changes on ppc64le (using docker multiarch). Before the changes, 10 GNU Radio tests out of 265 fail, and afterwards all tests pass. 🎉

[argilo]

I also tested Gqrx, and it runs normally on ppc64le with these changes in place.

[jdemel]

LGTM. Thanks for taking the time to fix this issue and for being persistent on it.
I feel like this is a solution that can work for existing users as well as new users on more architectures.

[jj1bdx]

volk_32fc_x2_s32fc_multiply_conjugate_add_32fc (not used in GNU Radio)

This kernel has been in use on https://github.com/jj1bdx/airspy-fmradion for a long time. Thanks for the fix!

[argilo]

You're welcome!

Is the new version working well for your application?

[argilo]

I updated the sample code above to include the exact VOLK version number which includes the fixes.

[jj1bdx]

@argilo After a few hours of trial, I have to conclude that:

• volk_32fc_x2_s32fc_multiply_conjugate_add_32fc of VOLK v3.1.0 worked OK on macOS 14.2 (Homebrew) with airspy-fmradion MultipathFilter::update_coeff().
• Either volk_32fc_x2_s32fc_multiply_conjugate_add_32fc or volk_32fc_x2_s32fc_multiply_conjugate_add2_32fc of VOLK v3.1.0 did not work (no compilation error, but the calculation failed), in generic kernel (no CPU optimization) on airspy-fmradion MultipathFilter::update_coeff() running on Ubuntu 22.04.3 x86_64 compiled with gcc version 11.4.0. I had to fall back to VOLK 3.0.0, which worked OK. Also, airspy-fmradion compiled with VOLK 2.5.1 of Ubuntu 22.04.3's apt repository worked OK.

[argilo]

Could you try reverting this commit (git revert e77c668946ed3d6662907895788ce13404021ea0) on VOLK 3.1.0 to see whether it reliably fixes the problem?

If not, perhaps you could try doing a git bisect between v3.0.0 and v3.1.0 to see which commit causes airspy-fmradion to stop working.

[jj1bdx]

@argilo

Your suggestion of reverting e77c668 worked (no calculation issue happened) on compiling and running Ubuntu 22.04.3 x86_64. In short, your suggestion in #695 (comment) worked.

[argilo]

I'll have a look over e77c668 to see if there is anything that could explain the problem.

What behaviour are you seeing from the volk_32fc_x2_s32fc_multiply_conjugate_add_32fc kernel when it misbehaves?

[jj1bdx]

The symptom I see in 3.1.0 is that the (complex) absolute value of each element of the computed result vector rapidly converges to zero, which should not happen in the normal calculation.

This is the code I've been using volk_32fc_x2_s32fc_multiply_conjugate_add_32fc kernel:
https://github.com/jj1bdx/airspy-fmradion/blob/65e6b5bb00b43061fa68d5999c80b0fdbd8247b8/sfmbase/MultipathFilter.cpp#L100-L143

[argilo]

I tried running airspy-fmradion inside valgrind, and valgrind reported many errors due to use of uninitialized memory. I wonder if that could be causing trouble.

[argilo]

The errors occur because m_save_value is not initialized when it is passed into VOLK. Could you try initializing it to zero in the PhaseDiscriminator constructor to see if that changes the behaviour?

[jj1bdx]

@argilo
the bug was fixed by applying your pull request jj1bdx/airspy-fmradion#43 - many thanks!

[argilo]

I'm glad to hear it! Thanks for sharing the results of your testing.

[jj1bdx]

@argilo I had to fix another bug for the stable operation of the adaptive multipath filter in airspy-fmradion. This is not related to this issue but rather an irregular/illegal value handling during the calculation. Details: jj1bdx/airspy-fmradion#42 (comment)