Skip to content

fix a small bug of the copying history samples in taps_5-8 processing of FIRF32 #245

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AlbertHuang-CPU
Copy link
Contributor

The number of history samples being copied in arm_fir_f32_5_8_mve should be numTaps-1 (see the file head comment and arm_fir_f32_1_4_mve as reference). So, the "blkCnt = numTaps" should be changed to "blkCnt = numTaps -1". However, to save cycles, I'd rather to change the next line from "blkCnt > 0" to "blkCnt > 1". This has passed the tests and the cycles reduced indeed.
In fact, the vectorization can be used here to achieve more performance uplift but there will be a little code size increase. So I'd just request this small change this time.

@christophe0606
Copy link
Contributor

@AlbertHuang-CPU I think the test must be improved. It is not strong enough to detect this kind of problem.
I have quickly looked at the main fir f32 implementation and I think there may be a similar problem too (or we are missing a detail regarding the implementation).

I'll upgrade the test first.

@christophe0606 christophe0606 added the bug Something isn't working label Mar 21, 2025
@AlbertHuang-CPU
Copy link
Contributor Author

Yes, there is a similar issue. I added the fix to the main fir f32 just now.
This kind of problem is not detectable by usual testing method because the corrupted data doesn't cause any issue since it will be overwritten by a right data of the next block. We can consider to integrate some tools into the test flow to do static analysis, or make some kind of fuzz testing or integration testing with scenario of sharing the memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants