arm_mult_q31: saturate to 0x7FFFFFFF, as the other two mult kernels do - #330
Open
DanielMBouyou wants to merge 1 commit into
Open
arm_mult_q31: saturate to 0x7FFFFFFF, as the other two mult kernels do#330DanielMBouyou wants to merge 1 commit into
DanielMBouyou wants to merge 1 commit into
Conversation
arm_mult_q31, arm_mult_q15 and arm_mult_q7 all document that results outside the allowable range are saturated. arm_mult_q15 and arm_mult_q7 shift by the full fractional width and then saturate. arm_mult_q31 shifts by 32, saturates to 31 bits and doubles, so its reachable ceiling is 0x7FFFFFFE. The only product that leaves the Q31 range, -1.0 x -1.0, therefore returns 0x7FFFFFFE where the documentation promises 0x7FFFFFFF. TEST_MULT_Q31_29 already loads 0x80000000 into both inputs and already compares against a reference pattern holding 0x7FFFFFFF. It passed only because ABS_ERROR_Q31 is 4, which absorbs the one LSB miss. The assertion is tightened to exact equality for that saturation case alone; ABS_ERROR_Q31 is left untouched and every other test keeps it. Stated because the diff understates it: the committed form forces the low bit of the result to zero, so the change is not confined to that one input pair. Measured over 1000000 uniform random pairs, running both kernels in one binary, 500079 outputs change, each by exactly one LSB, difference always within [0, 1]. Whether that accuracy is worth the cost on Cortex-M is a call for the maintainers; no cycle cost was measured here. Built and tested on x86-64 only. Nothing was run on an Arm target.
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.
There are two separate things in this PR and I would rather split them myself than
have them read as one. Part A is the defect: a saturation the documentation promises
and that does not happen. Part B is the full effect of the fix, which is larger than
the defect and is a trade-off that belongs to you, not to me.
Part A: the defect
A1. The test is already there, and the reference already holds the right value
Testing/Source/Tests/BasicTestsQ31.cpp,TEST_MULT_Q31_29("Test mul sat") loadsMAXNEG2_Q31_ID, which is0x80000000, into both inputs, and compares againstREF_POSSAT_16_Q31_ID. That reference comes fromTesting/PatternGeneration/BasicMaths.py:and
Tools.to_q31clamps on the way out:so the pattern committed to the repository holds
0x7FFFFFFF. The kernel returns0x7FFFFFFE. The test passes because the assertion isA four-LSB tolerance on a saturation case absorbs a one-LSB miss. Nothing about the
expected value is in dispute: it is already in your pattern.
This PR tightens that one assertion to exact equality, for the saturation case only.
ABS_ERROR_Q31is untouched and the otherarm_mult_q31tests keep it.A2. All three kernels of the family carry the same sentence
Verbatim, in
arm_mult_q31.c,arm_mult_q15.candarm_mult_q7.c:A3. The two siblings honour it
arm_mult_q15andarm_mult_q7shift by the full fractional width, then saturate:Checked against an integer model derived from the documented Q format, compared with
==:arm_mult_q7arm_mult_q15A4. One input pair in the whole domain is concerned
Q1.31 x Q1.31 leaves the binary point at bit 62. Getting back to Q1.31 is a right shift
of 31. The committed code shifts by 32, which puts the binary point at bit 30, saturates
to 31 bits, which is
2^30 - 1, and then doubles:so the reachable ceiling is
2^31 - 2. The only product that leaves the Q31 range is-1.0 x -1.0 = +1.0, that is0x80000000 x 0x80000000; every other pair fits. Measured:I built twelve out-of-range cases across eleven q31 kernels that document saturating
arithmetic, and
arm_scale_q31carries two of them. Before this patch, nine of thetwelve returned
0x7FFFFFFF, one did not, this one, and two could not be driven out ofrange at all, so they are recorded as untested rather than as passing: 9 + 1 + 2 = 12.
After this patch: ten saturate, none fail, the same two remain untested.
The replacement is the idiom already used elsewhere in the tree, including in the
non-
ARM_MATH_DSPbranch ofarm_cmplx_mult_real_q31.c:Part B: what the fix actually changes, which is much more than that one pair
Part A is about one input pair. The patch is not. I am stating this myself because
the diff would otherwise understate it.
out = (p >> 32); out = __SSAT(out, 31); out << 1produces2 * floor(p / 2^32).That value is always even. The correctly truncated Q31 result is
floor(p / 2^31),which is odd for half the domain. So the committed kernel loses the least significant
bit by construction, and the patch gives it back.
Measured by running the committed kernel and the patched kernel side by side in one
binary, over the same inputs. Both numbers come from compiled CMSIS-DSP code, and nothing
is modelled:
So: about half of all outputs change, each by exactly one LSB, never more, never in the
other direction. On an edge-weighted input set (every container edge against every other,
plus random) the figure was 26 251 changed out of 54 225, and after the patch that set
matches the exact integer model at every one of its 54 225 points, where before it matched
at 27 974.
This is accuracy traded for speed, and the trade is very probably deliberate.
(a*b) >> 32followed by<< 1is the shape that falls onto a single most-significant-wordmultiply on Cortex-M; the replacement needs the full 64-bit product and a shift across the
register pair.
I have measured no cycle cost, on any core, and I claim none. I have no way to weigh
one LSB of accuracy in a vector multiply against instructions in a hot loop on the parts
you care about. That balance is yours.
If you want the current form kept, then the correct change is the opposite of this PR:
document the exception in the three files, the way #327 documented the missing saturation
in
arm_pid_q31, and close this. I will follow either way, and I would rather you told methan merged this because it was quiet.
Spotted, not fixed
arm_cmplx_mult_real_q31.ccarries both idioms inside the same function, selected byARM_MATH_DSP, and so returns two different numbers for the same inputs:Same documented sentence as above. Deliberately left untouched here: it is a separate
decision, and this change is meant to stay small.
Where this was run
An x86-64 Linux container: gcc 12.2.0,
cmake -S . -B build-host -DHOST=ON -DDISABLEFLOAT16=ON, and theTesting/framework built natively.Nothing was run on an Arm target: no board, no FVP, no simulator of any kind.
Someone with hardware should confirm before this is merged. That applies to the numbers in
Part B as much as to Part A.
One note on running
Testing/off Arm:Testing/CMakeLists.txthardcodes-march=armv8.2-a+fp16fmlat three sites, so as committed it only builds on an AArch64host. I dropped that flag locally to build it on x86-64. That is a local build-side change
only; it is not part of this PR and it touches no library or test source.
Results after the change:
Testing/, 19 suites: 502 tests passed, 1 failed, namelytest_mat_qr_f32, SNR87.0749 < 90. That one fails identically with and without this patch; I re-ran the suite
with
arm_mult_q31.crestored to its committed content to check, rather than assume.PythonWrapper/examples/testall.py: 309 tests, 4 failures, allpre-existing and all floating point:
test_cholesky_f32,test_nuttall4,test_nuttall4a,test_nuttall4b.MFCCQ31passes;arm_mfcc_q31.c:141is the only in-tree caller ofarm_mult_q31, soit is the one place where the Part B change propagates inside the library.
-Wall -Wextra -Wsign-compare -Wdouble-promotion -Wfloat-conversion -Wmissing-prototypes.