docs(q31): correct false '...saturated to 1.31 format' claim in 9 files - #345
Open
saikumar-mandaji wants to merge 1 commit into
Open
Conversation
Following the pattern of ARM-software#327/PR (arm_pid_q31), these nine files' 'Scaling and Overflow Behavior' doc comments claim the final 2.62 -> 1.31 narrowing conversion saturates. It does not: in every case the scalar path narrows with a bare (q31_t) cast (a right shift then truncating cast, e.g. '(q31_t) (sum >> 31)' or '*pOut++ = (q31_t) acc;'), with no call to a saturating helper such as clip_q63_to_q31 anywhere in that narrowing step. A caller relying on the documented saturation guarantee to bound worst-case output would instead observe silent wraparound. Verified per-file, not just grepped from the issue: for arm_lms_q31.c and arm_lms_norm_q31.c in particular, clip_q63_to_q31() does appear in the file, but only in the coefficient-update path (LMS tap adaptation), never on the accumulator-to-output narrowing the doc comment describes -- so the same fix applies there too, not just to the files with zero clip_q63_to_q31 occurrences. Scope note (matching ARM-software#328, which lists these same nine files): this covers the documented scalar-path conversion only. The MVEI/Helium and NEON kernels narrow through vector intrinsics rather than a (q31_t) cast, so they're out of scope here -- I made no attempt to check whether Helium's narrowing actually saturates, consistent with what ARM-software#328 already flagged as unverified. arm_mat_cmplx_mult_q31.c is intentionally excluded: its scalar path genuinely calls clip_q63_to_q31 on the output, so its existing doc wording is correct. Nothing was compiled or executed for this change -- it edits comment text only, no code paths are touched, so there's nothing for a compiler to meaningfully check beyond confirming the comment blocks still parse (verified by eye and via 'git diff' review, since no compiler flags a malformed /** */ block as anything but a warning at best). Fixes ARM-software#328
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
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.
Bug
Following the pattern already accepted in #327 (
arm_pid_q31, merged), these nine files'Scaling and Overflow Behaviordoc comments claim the final 2.62 -> 1.31 narrowing conversion saturates. It does not: in every case the scalar path narrows with a bare(q31_t)cast (a right shift then truncating cast, e.g.(q31_t) (sum >> 31)or*pOut++ = (q31_t) acc;), with no call to a saturating helper such asclip_q63_to_q31anywhere in that narrowing step. A caller relying on the documented saturation guarantee to bound worst-case output would instead observe silent wraparound. See #328, which enumerated exactly these nine files.I independently re-verified each file rather than trusting the issue's grep-based classification at face value. Notably,
arm_lms_q31.candarm_lms_norm_q31.cdo callclip_q63_to_q31()— but only in the coefficient-update path (LMS tap adaptation), never on the accumulator-to-output narrowing the doc comment actually describes, so the same fix applies there too, not just to the files with zeroclip_q63_to_q31occurrences.arm_mat_cmplx_mult_q31.c(also mentioned in #328) is intentionally excluded: its scalar path genuinely callsclip_q63_to_q31on the output, so its existing doc wording is already correct.Fix
Reword each doc comment from "...saturated to 1.31 format..." to "...converted to 1.31 format..." plus an explicit "This conversion is not saturating." line, mirroring the exact wording pattern used in the already-merged #327.
Scope
This covers the documented scalar-path conversion only. The MVEI/Helium and NEON kernels narrow through vector intrinsics rather than a
(q31_t)cast, so I made no attempt to check whether Helium's narrowing actually saturates — consistent with what #328 already flagged as unverified and out of scope for this PR.Verification
This is a comment-only change — no code paths are touched, so there's nothing for a compiler to meaningfully check. Verified by reading each file's actual narrowing site (see above) and by
git diffreview that all nine/** */blocks remain well-formed.Fixes #328