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

Complex max_index const correctness #782

Open
jsallay opened this issue Jan 20, 2025 · 3 comments
Open

Complex max_index const correctness #782

jsallay opened this issue Jan 20, 2025 · 3 comments

Comments

@jsallay
Copy link
Contributor

jsallay commented Jan 20, 2025

I noticed that the 32fc_index_max/min kernels take in a non-const pointer:

static inline void volk_32fc_index_max_32u_a_avx2_variant_0(uint32_t* target,
                                                            lv_32fc_t* src0,
                                                            uint32_t num_points)

I think this is just an oversight. I should be able to submit a patch in the next few days.

@jdemel
Copy link
Contributor

jdemel commented Jan 22, 2025

I assume you'd want to update to:

static inline void volk_32fc_index_max_32u_a_avx2_variant_0(uint32_t* target,
                                                            const lv_32fc_t* src0,
                                                            const uint32_t num_points)

I agree, this was probably an oversight. It could be related to very old API design as well. Answering this conclusively might require some code archeology.

The questions that come up:

  • does this change API?
    • should not be a problem because adding const should have no effect.
  • does this change ABI?
    • I'd like to clarify this.

@tomneda
Copy link

tomneda commented Jan 22, 2025

Same applies to

void volk_32fc_x2_square_dist_32f(float* target, lv_32fc_t* src0, lv_32fc_t* points,
unsigned int num_points)

and

void volk_32fc_x2_s32f_square_dist_scalar_mult_32f(float* target, lv_32fc_t* src0,
lv_32fc_t* points, float scalar, unsigned int num_points) 

where src0 and points should be const .

@jsallay
Copy link
Contributor Author

jsallay commented Jan 25, 2025

From my cursory googling, it looks like the internet says that this is an ABI compatible change. Const is a compiler construct that doesn't exist in the actual executable. So (unlikely) optimizations may be in the new executable because it knows the array is const, but the interface should be identical.

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

No branches or pull requests

3 participants