fix(none.h): remove implementation-defined signed shifts in clip helpers (#211)#321
Open
deekshaNVIDIA wants to merge 1 commit into
Open
fix(none.h): remove implementation-defined signed shifts in clip helpers (#211)#321deekshaNVIDIA wants to merge 1 commit into
deekshaNVIDIA wants to merge 1 commit into
Conversation
The clip_q63_to_q31/clip_q63_to_q15/clip_q31_to_q7/clip_q31_to_q15 software fallbacks detected saturation by right-shifting a signed, possibly-negative value. Per C99 6.5.7/5 the result of right-shifting a negative signed integer is implementation-defined, which static analysers (e.g. MISRA) flag. Replace the shift-based logic with equivalent, fully-defined range checks. Behaviour is preserved bit-for-bit: verified by an exhaustive sweep over all 2^32 int32 inputs (q31->q7/q15) and 2x10^8 structured/random 64-bit inputs (q63->q31/q15), 0 mismatches. The clip_q31_to_q7 threshold (signed 24-bit range, not the Q7 range) and the clip_q63_to_q15 bit selection are kept exactly as before. Fixes ARM-software#211
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #211
Problem
The four software-fallback clip helpers in
Include/dsp/none.h(
clip_q63_to_q31,clip_q63_to_q15,clip_q31_to_q7,clip_q31_to_q15)detect saturation by right-shifting a signed, possibly negative value, e.g.:
Per C99 §6.5.7/5, right-shifting a negative signed integer yields an
implementation-defined result. These are exactly the lines flagged in #211
and by static analysers (e.g. MISRA).
Fix
Replace the shift-based detection with equivalent, fully-defined range checks
using the existing
Q31_MAX/Q31_MIN/Q15_MAX/Q15_MIN/Q7_MAX/Q7_MINmacros from
arm_math_types.h. No signed right shifts remain; the only shiftleft is an unsigned one in
clip_q63_to_q15.Behaviour is preserved exactly (this is a portability/UB cleanup, not a
functional change). Two quirks of the originals are deliberately kept verbatim
so nothing changes for existing callers:
clip_q31_to_q7saturates outside the signed 24-bit range (±2^23), notthe Q7 range — preserved via the
0x007FFFFF/0x00800000thresholds.clip_q63_to_q15returns bits[30:15]of the in-range value — preserved via(q15_t)((uint64_t)x >> 15), whose low 16 bits are identical to the oldsigned shift.
(If either quirk is actually an unintended bug, I'm happy to address it in a
separate PR — I kept this one strictly behaviour-preserving.)
Verification
Bit-exact equivalence, real compiler (clang 22, C11): the verbatim
originals vs. the rewrites over
int32inputs forclip_q31_to_q7andclip_q31_to_q15, andclip_q63_to_q31andclip_q63_to_q15.Result: 0 mismatches.
The edited
none.hcompiles cleanly (no-DSP host config,-Wall -Wextra -Wconversion -Wsign-conversion -Wshadow, no warnings).Only
clip_q63_to_q31andclip_q31_to_q15are called inside the librarytoday (e.g.
arm_recip_q31,arm_div_int64_to_int32, and several Q31 Sourcefiles); both are among the exhaustively/structurally verified functions.
Happy to add a dedicated unit test for these helpers if you'd like.