Skip to content

ASE: mathutils.hh: improve fast_log2, add fast_log2_block#64

Open
swesterfeld wants to merge 1 commit into
tim-janik:trunkfrom
swesterfeld:fast-log2-updates
Open

ASE: mathutils.hh: improve fast_log2, add fast_log2_block#64
swesterfeld wants to merge 1 commit into
tim-janik:trunkfrom
swesterfeld:fast-log2-updates

Conversation

@swesterfeld

Copy link
Copy Markdown
Collaborator
  • avoid using a union in fast_log2 (fix undefined behaviour)
  • make code auto vectorizable by gcc
  • add a block version for fast_log2 (should be auto vectorized)
  • assume sign bit == 0, makex exponent extraction easier

 - avoid using a union in fast_log2 (fix undefined behaviour)
 - make code auto vectorizable by gcc
 - add a block version for fast_log2 (should be auto vectorized)
 - assume sign bit == 0, makes exponent extraction easier

Signed-off-by: Stefan Westerfeld <stefan@space.twc.de>
@tim-janik

Copy link
Copy Markdown
Owner

@greptile

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Overview

Greptile Summary

This PR improves the fast_log2 function in ase/mathutils.hh by addressing undefined behavior and enhancing vectorization capabilities. The main changes replace union-based type punning with standards-compliant memcpy operations for float-to-int bit reinterpretation. The original implementation used a union to directly access float bits as integers, which violates C++ strict aliasing rules and constitutes undefined behavior, even though it worked in practice.

The refactored code maintains the same mathematical approach - extracting the IEEE 754 exponent and manipulating the mantissa - but uses memcpy for type conversion, which modern compilers optimize away while remaining standards-compliant. The implementation assumes positive input values (sign bit = 0), simplifying bit manipulation by avoiding sign checks. Additionally, a new fast_log2_block function processes arrays of values in a loop structure that compilers can auto-vectorize, improving performance for bulk audio processing operations common in DSP applications.

These changes integrate well with the codebase's audio synthesis engine (ASE), where fast logarithmic operations are crucial for real-time audio processing, parameter mapping, and frequency domain calculations.

Important Files Changed

Changed Files
Filename Score Overview
ase/mathutils.hh 5/5 Refactored fast_log2 to use memcpy instead of union type punning, fixing undefined behavior while maintaining performance and adding vectorizable block function

Confidence score: 5/5

  • This PR is safe to merge with minimal risk as it fixes undefined behavior while preserving functionality
  • Score reflects well-understood changes that replace problematic code with standards-compliant equivalents without altering the mathematical logic
  • No files require special attention as the changes are localized to a single header with clear improvements

1 file reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment thread ase/mathutils.hh
const int EXPONENT_MASK = 0x7F800000;
int iv;
memcpy (&iv, &value, sizeof (float)); // iv = *(int *) &value
int fexp = (iv >> 23) - FloatIEEE754::BIAS; // extract exponent without bias (rely on sign bit == 0)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: The comment 'rely on sign bit == 0' indicates this function assumes positive inputs only. Consider adding input validation or documenting this constraint in the function comment.

Prompt To Fix With AI
This is a comment left during a code review.
Path: ase/mathutils.hh
Line: 108:108

Comment:
**style:** The comment 'rely on sign bit == 0' indicates this function assumes positive inputs only. Consider adding input validation or documenting this constraint in the function comment.

How can I resolve this? If you propose a fix, please make it concise.

Comment thread ase/mathutils.hh
* function are written in a way that both, gcc and clang should auto vectorize it
*/
extern inline void
fast_log2_block (float *values, int n_values)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Parameter should be const float *values since the input values are being read, not the pointer itself being modified.

Prompt To Fix With AI
This is a comment left during a code review.
Path: ase/mathutils.hh
Line: 133:133

Comment:
**style:** Parameter should be `const float *values` since the input values are being read, not the pointer itself being modified.

How can I resolve this? If you propose a fix, please make it concise.

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

Successfully merging this pull request may close these issues.

2 participants