Skip to content

Audio: Use generic saturation logic for improved efficiency #9170

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.

Sign up for GitHub

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

Closed
ShriramShastry wants to merge 1 commit into from
Closed

Audio: Use generic saturation logic for improved efficiency #9170

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 67 additions & 25 deletions src/include/sof/audio/format_generic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/* SPDX-License-Identifier: BSD-3-Clause
*
* Copyright(c) 2021 Intel Corporation. All rights reserved.
*
* Author: Shriram Shastry <malladi.sastry@linux.intel.com>
*/

#ifndef __SOF_AUDIO_FORMAT_GENERIC_H__
Expand All @@ -9,45 +11,85 @@
#include <stdint.h>

/* Saturation inline functions */

/**
* @brief Saturate an int64_t value to fit within the range of int32_t.
*
* This function ensures that the input value 'x' fits within the range of
* int32_t. If 'x' exceeds this range, it will be bound to INT32_MAX or INT32_MIN
* accordingly.
*
* @param x The input value to saturate.
* @return int32_t The saturated int32_t value.
*/
static inline int32_t sat_int32(int64_t x)
{
if (x > INT32_MAX)
return INT32_MAX;
else if (x < INT32_MIN)
return INT32_MIN;
else
return (int32_t)x;
int64_t mask_overflow = (INT32_MAX - x) >> 63;
int64_t mask_underflow = (x - INT32_MIN) >> 63;
Copy link
Collaborator

@marc-hb marc-hb Jul 8, 2024

Choose a reason for hiding this comment

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

When x is big enough then (x - INT32_MIN) overflows, which is undefined behavior because it's signed.

Same overflow issue above when x is negative enough.

Signed overflow is undefined behavior:
https://wiki.sei.cmu.edu/confluence/display/c/INT32-C.+Ensure+that+operations+on+signed+integers+do+not+result+in+overflow

Code like this is being banned from the Linux kernel right now for security reasons:
https://lwn.net/Articles/979747/

There are compiler flags to define it but that would introduce a very subtle and hard to track dependency. For what benefit?
https://www.gnu.org/software/c-intro-and-ref/manual/html_node/Signed-Overflow.html

Even ignoring undefined behavior, this code is much, much harder to follow than the previous one.

Can branch prediction ever matter for code that small?

Copy link
Contributor Author

@ShriramShastry ShriramShastry Jul 9, 2024

Choose a reason for hiding this comment

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

Can branch prediction ever matter for code that small?

Yes- Please take a look at the low-level impacts through assembly comparision

The bitwise approach uses fewer branching instructions, making it more efficient as branch prediction may fail and cause pipeline stalls.

image

Bitwise Approach: Uses fewer branches, reducing the risk of pipeline stalls and improving efficiency.
Complexity: Despite appearing complex, bitwise operations provide stability and avoid undefined behavior from signed overflows.
Branch Prediction Impact: Even small code sections can significantly benefit from reduced branching

Copy link
Member

@lgirdwood lgirdwood Jul 9, 2024

Choose a reason for hiding this comment

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

@ShriramShastry agree this should improve branch prediction and instruction pipeline throughput, but I think its best to show a real world test i.e. take a piece of FW code that is a heavy user of these APIs and place some timestamps around usage. The timestamps can then be measured before/after the changes with a real workload to show improvement.

Copy link
Contributor Author

@ShriramShastry ShriramShastry Jul 9, 2024

Choose a reason for hiding this comment

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

Sure

Copy link
Collaborator

@marc-hb marc-hb Jul 9, 2024

Choose a reason for hiding this comment

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

Complexity: Despite appearing complex, bitwise operations provide stability and avoid undefined behavior from signed overflows.

I don't think you understand what "undefined behavior" means. This is not a problem that shows up in assembly code.

Copy link
Contributor Author

@ShriramShastry ShriramShastry Jul 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for your feedback. Marc.

I thank your attention to the complexity and stability provided by bitwise operations. However, I believe there might be a misunderstanding regarding the definition and implications of "undefined behavior" (UB) in the context of signed integer overflow.

Explanation:
Undefined Behavior (UB):

I went over the links shared above, In C and C++, "undefined behavior" refers to a situation where the language specification does not define what should happen. This can lead to unpredictable results as the compiler is free to generate any code for UB constructs.
A classic case of UB in C is signed integer overflow. For example, adding two large int32_t values that exceed the range of int32_t can result in UB because the C standard does not define the outcome of overflow for signed integers. https://wiki.sei.cmu.edu/confluence/display/c/INT32-C.+Ensure+that+operations+on+signed+integers+do+not+result+in+overflow#NoncompliantCodeExample

Bitwise Operations:
The bitwise operations used in the provided code ensure that values are clamped/bounded within their respective ranges without causing overflow, thereby ensuring predictable behavior.

For example, the code for sat_int32:

static inline int32_t sat_int32(int64_t x) {
    int64_t mask_overflow = (INT32_MAX - x) >> 63;
    int64_t mask_underflow = (x - INT32_MIN) >> 63;

    x = (x & ~mask_overflow) | (INT32_MAX & mask_overflow);
    x = (x & ~mask_underflow) | (INT32_MIN & mask_underflow);
    return (int32_t)x;
}

Here, bitwise operations are used to safely bound int64_t values to the int32_t range, avoiding the issues of signed overflow.

Understanding in Assembly:
While undefined behavior often has unpredictable and compiler-dependent outcomes, bitwise operations tend to translate directly to assembly without causing UB.
Ensuring signed overflows are handled in C code avoids reliance on compiler-specific behavior and promotes consistent, predictable execution across different platforms.

Summary:
Bitwise operations provide a robust way to handle out-of-range values, preventing undefined behavior stemming from signed integer overflow in C. This ensures predictable, stable, and portable code execution.

If there is an aspect I might have overlooked regarding the specific implications on assembly code or if you meant something different by undefined behavior, please let me know, and I can address it further.

Copy link
Collaborator

@marc-hb marc-hb Jul 18, 2024

Choose a reason for hiding this comment

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

The bitwise operations used in the provided code ensure that values are clamped/bounded within their respective ranges without causing overflow, thereby ensuring predictable behavior.

static inline int32_t sat_int32(int64_t x) {
    int64_t mask_overflow = (INT32_MAX - x) >> 63;

This is not true because the shift happens after the undefined behavior.

Copy link
Collaborator

@marc-hb marc-hb Jul 18, 2024

Choose a reason for hiding this comment

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

Can branch prediction ever matter for code that small?

Yes- Please take a look at the low-level impacts through assembly comparison

Assembly differences are interesting but they're not answering the question. The question was "Does this matter? (and how much)". For this sort of optimization to matter you need:

  1. A branch predictor
  2. A branch prediction which is difficult to make and often enough wrong
  3. A long enough pipeline
  4. Code in the critical path

Most performance optimizations tend to be very "cruel" because they're correct in theory and they don't matter in practice. That's what Donald Knuth meant when he wrote "premature optimization is the root of all evil" https://en.wikiquote.org/wiki/Donald_Knuth


x = (x & ~mask_overflow) | (INT32_MAX & mask_overflow);
x = (x & ~mask_underflow) | (INT32_MIN & mask_underflow);
return (int32_t)x;

}

/**
* @brief Saturate an int32_t value to fit within the range of a 24-bit integer.
*
* This function ensures that the input value 'x' fits within the range of a
* 24-bit integer. If 'x' exceeds this range, it will be bound to INT24_MAXVALUE
* or INT24_MINVALUE accordingly.
*
* @param x The input value to saturate.
* @return int32_t The saturated int32_t value.
*/
static inline int32_t sat_int24(int32_t x)
{
if (x > INT24_MAXVALUE)
return INT24_MAXVALUE;
else if (x < INT24_MINVALUE)
return INT24_MINVALUE;
else
return x;
int32_t mask_overflow = (INT24_MAXVALUE - x) >> 31;
int32_t mask_underflow = (x - INT24_MINVALUE) >> 31;

x = (x & ~mask_overflow) | (INT24_MAXVALUE & mask_overflow);
x = (x & ~mask_underflow) | (INT24_MINVALUE & mask_underflow);
return x;
}

/**
* @brief Saturate an int32_t value to fit within the range of a 16-bit integer.
*
* This function ensures that the input value 'x' fits within the range of a
* 16-bit integer. If 'x' exceeds this range, it will be bound to INT16_MAX or INT16_MIN
* accordingly.
*
* @param x The input value to saturate.
* @return int16_t The saturated int16_t value.
*/
static inline int16_t sat_int16(int32_t x)
{
if (x > INT16_MAX)
return INT16_MAX;
else if (x < INT16_MIN)
return INT16_MIN;
else
return (int16_t)x;
int32_t mask_overflow = (INT16_MAX - x) >> 31;
int32_t mask_underflow = (x - INT16_MIN) >> 31;

x = (x & ~mask_overflow) | (INT16_MAX & mask_overflow);
x = (x & ~mask_underflow) | (INT16_MIN & mask_underflow);
return (int16_t)x;
}

/**
* @brief Saturate an int32_t value to fit within the range of an 8-bit integer.
*
* This function ensures that the input value 'x' fits within the range of an
* 8-bit integer. If 'x' exceeds this range, it will be bound to INT8_MAX or INT8_MIN
* accordingly.
*
* @param x The input value to saturate.
* @return int8_t The saturated int8_t value.
*/
static inline int8_t sat_int8(int32_t x)
{
if (x > INT8_MAX)
return INT8_MAX;
else if (x < INT8_MIN)
return INT8_MIN;
else
return (int8_t)x;
int32_t mask_overflow = (INT8_MAX - x) >> 31;
int32_t mask_underflow = (x - INT8_MIN) >> 31;

x = (x & ~mask_overflow) | (INT8_MAX & mask_overflow);
x = (x & ~mask_underflow) | (INT8_MIN & mask_underflow);
return (int8_t)x;
}

#endif /* __SOF_AUDIO_FORMAT_GENERIC_H__ */