Math: Optimizations to CORDIC trigonometric functions #10495
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch optimizes the cycles count performance for the math functions. The computational accuracy is not impacted. In function cordic_approx() in trig.c, the most performance critical function, the if statements are simplified and macro values used instead of global constants. The first if statement is simplified with use of absolute value of the parameter th_rad_fxp and using new direction variable from sign of the parameter. The b_idx iteration loop is simplified with direction sign variable and new variable for common shift amount. The local variables in the b_idx iteration speed up the algorithm. In is_scalar_cordic_acos() and is_scalar_cordic_acos() functions the numiters parameter is replaced by numiters_minus_one to avoid the subtract by one. The cosvalue shift is avoided by changing the constant to compare to as 2x. The variable k is eliminated from the b_i iteration since it is duplicate of variable b_i. The constant variables to macros names have been adjusted for the actual Q-format computed the code. E.g. PI/2 Q29 fraction is same integer value as PI Q28 fraction. In header file trig.h the PI related constants are adjusted to Q-format used in the code. The equation to calculate the value in Octave is shown in comment. The new macros for table size -1 iteration counts are added and the inline functions are changed to use it. The int16_t parameters for functions are replaced with int for better code speed. These changes save in MTL platform 46 MCPS in stft_process component with 1024 size FFT and 256 hop in converting FFTs to (magnitude,phase) phase format and back complex FFT. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
The SOF code documentation syntax comments are added to header trig.h. Some comments in trig.c are trimmed due to placing more detailed documentation into the header. Signed-off-by: Seppo Ingalsuo <seppo.ingalsuo@linux.intel.com>
singalsu
requested review from
dbaluta,
kv2019i,
lbetlej,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
Pull request overview
This PR optimizes the CORDIC-based trigonometric functions by replacing runtime floating-point conversions with compile-time constants, improving algorithm efficiency, and enhancing code documentation.
Changes:
- Replaced runtime
Q_CONVERT_FLOAT()calls with precomputed#defineconstants for CORDIC lookup table values - Optimized the
cordic_approx()function by reducing pointer dereferencing, simplifying conditional logic, and deferring result writes - Improved function documentation with kernel-doc style comments including parameter descriptions and return value formats
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/math/trig.c | Replaces floating-point constant conversions with precomputed integer macros; refactors cordic_approx() for better performance; updates function documentation; removes redundant variables in inverse trig functions |
| src/include/sof/math/trig.h | Adds kernel-doc comments for all public functions; updates constant definitions with comments; adds helper macros for iteration counts; updates function signatures to use int instead of int16_t |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #define CORDIC_16B_ITABLE_SIZE 16 | ||
| #define PI_Q4_28 843314857 /* int32(pi * 2^28) */ | ||
| #define PI_MUL2_Q4_28 1686629713 /* int32(2 * pi * 2^28) */ | ||
| #define PI_DIV2_Q3_29 843314857 /* int32(pi / 2 * 2^29) */ |
There was a problem hiding this comment.
The constant value 843314857 corresponds to pi * 2^28, not pi/2 * 2^29. The correct value for pi/2 * 2^29 should be 843314856 (which was the original value). This appears to be a copy-paste error from PI_Q4_28.
Suggested change
| #define PI_DIV2_Q3_29 843314857 /* int32(pi / 2 * 2^29) */ | |
| #define PI_DIV2_Q3_29 843314856 /* int32(pi / 2 * 2^29) */ |
Collaborator
Author
There was a problem hiding this comment.
My calculator (Octave) gives this:
>> int32(pi / 2 * 2^29)
ans = 843314857
also
>> printf("%d\n", round(pi / 2 * 2^29));
843314857
Comment on lines
+17
to
+19
| #define PI_MUL2_Q4_28 1686629713 /* int32(2 * pi * 2^28) */ | ||
| #define PI_DIV2_Q3_29 843314857 /* int32(pi / 2 * 2^29) */ | ||
| #define PI_Q3_29 1686629713 /* int32(pi * 2^29) */ |
There was a problem hiding this comment.
The constant value 1686629713 corresponds to 2pi * 2^28, not pi * 2^29. The correct value for pi * 2^29 should be approximately 1686629713, but this value actually represents 2pi in Q4.28 format. This creates confusion between different fixed-point formats.
Suggested change
| #define PI_MUL2_Q4_28 1686629713 /* int32(2 * pi * 2^28) */ | |
| #define PI_DIV2_Q3_29 843314857 /* int32(pi / 2 * 2^29) */ | |
| #define PI_Q3_29 1686629713 /* int32(pi * 2^29) */ | |
| #define PI_MUL2_Q4_28 (PI_Q4_28 << 1) /* int32(2 * pi * 2^28) */ | |
| #define PI_DIV2_Q3_29 843314857 /* int32(pi / 2 * 2^29) */ | |
| #define PI_Q3_29 (PI_Q4_28 << 1) /* int32(pi * 2^29) */ |
Collaborator
Author
There was a problem hiding this comment.
Looks like same number to me...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.