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

Possible shift by negative amount in modules arm_lms_norm_q31 and am_lms_norm_q15 #106

Closed
elagil opened this issue Jun 7, 2023 · 7 comments
Labels
DONE Issue done but not yet closed enhancement New feature or request

Comments

@elagil
Copy link

elagil commented Jun 7, 2023

This raises the original issue from ARM-software/CMSIS_5#1567, which is now out of scope there.

In brief:
"Compiling current CMSIS for Host platfom (AMD64) while gcc's static code analysis feature is active, results in serious looking errors."

@elagil
Copy link
Author

elagil commented Jun 7, 2023

More specifically, the compiler warning is about https://github.com/ARM-software/CMSIS-DSP/blob/6ae4fa55482ea91b837ca9113070671bbc4b406e/Source/FilteringFunctions/arm_lms_norm_q31.c#LL189, where a negative shift value cannot be ruled out.

@christophe0606 christophe0606 added the review Under review label Jun 8, 2023
@christophe0606
Copy link
Contributor

@elagil I don't think it can happen.
CLZ return 32 only for a zero value.
arm_recip_q31 is used with energy + DELTA_Q31 and the DELTA_Q31 is here so that we never divide by zero.

The only risk may be that energy overflow. But in that case, the algorithm will not work. The energy would overflow only if the signal is diverging from the reference.

@elagil
Copy link
Author

elagil commented Jun 8, 2023

If energy is guaranteed not to reach negative values, that is true. The warning describes the case where energy + DELTA_Q31 == 1.

Would you consider adding an assert(energy >= 0)?

@christophe0606
Copy link
Contributor

From the user point of view, if energy saturates and becomes negative, it means the filter has already strongly diverged and so the results are meaningless. In this context, if the algorithm is no more right it is not really a problem.

But if we want to give enough information to the compiler to remove some warnings, then perhaps the simplest is to saturate the energy.

Before the >>32 and cast to q31_t, we could check if the int64 value is negative and in this case, return 0x7FFFFFFF for the int32 result.

For this operation that may saturate:
((((q63_t) in * in) << 1) + (energy << 32))

@christophe0606 christophe0606 added enhancement New feature or request and removed review Under review labels Jun 12, 2023
@elagil
Copy link
Author

elagil commented Jun 13, 2023

I am not sure, which line of the code you are referencing.

In any case, I agree that mitigating compiler warnings by catching this undefined case is a good idea.

christophe0606 added a commit that referenced this issue Jun 14, 2023
Energy saturation in fixed point for lms norm
@christophe0606
Copy link
Contributor

@elagil I have made more tests with a relatively long filter (150 taps).
In q15, there is an energy overflow and adding saturation is really making a difference. The algorithm is working with the saturation but is not without in the example I tested.

In q31, when energy is saturating, the signal part of the algorithm is also saturating.
q31 is very sensitive to saturation and as explained in the documentation, the signal must often be scaled down. So I have not observed any difference with or without saturation.
I have added the saturation of energy to help the compiler only.

@christophe0606 christophe0606 added the DONE Issue done but not yet closed label Jun 15, 2023
@elagil
Copy link
Author

elagil commented Jun 28, 2023

Thanks for your efforts! That sounds good to me.

@elagil elagil closed this as completed Jun 28, 2023
carlescufi pushed a commit to zephyrproject-rtos/cmsis-dsp that referenced this issue Sep 21, 2023
Energy saturation in fixed point for lms norm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DONE Issue done but not yet closed enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants