Commit 4a8f5b5
authored
Fix arm_transpose_conv_s8 row misalignment when pad_h % stride_h != 0 (#233)
## Summary
`arm_transpose_conv_s8()` streams input rows through a rolling circular
output-row buffer, flushing finished rows to the output as it goes. The
number of rows to flush after processing input row `j` was decided by a
binary gate:
```c
if (skip_rows_top == 0) { flush stride_y rows }
```
`skip_rows_top` is `MAX(0, pad_y - j * stride_y)`, i.e. it only becomes
zero once input row `j` has advanced past the top padding. Before that
point the gate flushes nothing at all — even on the specific row where
the padding region ends part-way through — so it never flushes a
*partial* count.
That's correct only when `pad_y % stride_y == 0`, because in that case
the row where padding ends coincides exactly with a full `stride_y`-row
boundary, so "flush 0 until `skip_rows_top==0`, then flush `stride_y`
every row" happens to be exactly right.
When `pad_y % stride_y != 0`, the transition row should flush a
*partial* count (between 1 and `stride_y - 1`), not 0. Skipping it
permanently shifts every later flush by one buffer position, so
subsequent input rows accumulate into the wrong rolling-buffer row, and
the deficit is never made up: one full output row is left completely
unwritten (still holding whatever the caller's buffer contained
beforehand), while other rows mix contributions meant for their
neighbour.
Fixes #230.
## Fix
Lines ~141-153 of `Source/ConvolutionFunctions/arm_transpose_conv_s8.c`:
replaced the binary gate with the exact number of rows that just became
final, using the same `MAX(0, ...)` padding-clip idiom already used
elsewhere in this function for `skip_rows_top`/`skip_rows_bottom`:
```c
rows_to_flush = MAX(0, (j + 1) * stride_y - pad_y) - MAX(0, j * stride_y - pad_y)
```
This telescopes to `stride_y` in steady state and to `0` while still
fully inside the padding region — exactly matching the old behaviour
whenever `pad_y % stride_y == 0` (provable algebraically, and confirmed
empirically below) — while correctly producing a partial count on the
transition row otherwise.
The identical binary-gate pattern also appeared in the `NULL`-bias reset
branch inside the same flush block; it has the same fix applied.
Also bumped the file's `$Date`/`$Revision` header (`V.2.0.0` ->
`V.2.0.1`) per the repo's versioning convention (`Version & Date`
section of the Contribution Guideline).
Horizontal (`pad_x`) misalignment is **not** affected by this bug and
needed no change: a full input row's horizontal accumulation completes
within a single `arm_nn_transpose_conv_row_s8_s32` call, so there is no
cross-call scheduling state on that axis. `transpose_conv_3` already
exercises `pad_x % stride_x != 0` and passes before and after this
change.
## Verification
All verification was done by compiling the real, unmodified library
source (not a reimplementation of the logic under test) standalone on
host with gcc, and running it directly — no simulator, no modification
to the Unity test harness.
1. **Reproduced the reported bug** against unmodified upstream
`arm_transpose_conv_s8.c` + `arm_nn_transpose_conv_row_s8_s32.c` using
the configuration from the issue (input 4x4x8, filter 4x4x4x8 (out_ch),
stride 2x2, pad 1x1, `pad % stride = 1`): **152/256 output elements
wrong (59.4%)**, matching the issue report exactly.
2. **Poison-fill test**: filled the output buffer with a `0xAA` sentinel
before calling the unmodified function. Output row 7 (the last of 8
rows) came back **entirely as sentinel bytes** — proof the function
returns having left part of the documented output tensor completely
unwritten, not merely numerically wrong.
3. **Independent reference implementation**: wrote a from-scratch
reference for transposed convolution using the direct "gather/scatter"
mathematical definition (a full accumulate-into-a-plain-array approach,
not derived from this file's rolling-buffer scheduling), reusing only
the library's own unmodified `arm_nn_requantize()` for rounding. This
reference was itself validated against ARM's own official
TensorFlow-generated golden data for the existing `transpose_conv_1`
unit test (nonzero input/output offsets, per-channel multiplier/shift,
batch size 2): **0/1944 elements differ**, establishing it as a
trustworthy correctness oracle independent of the third-party issue's
own attached golden file.
4. **With the fix applied**, library output on the reported
configuration matches the validated reference exactly: **0/256 elements
differ** (previously 152/256).
5. **No regressions** — re-ran all 4 existing official unit tests whose
`in_ch` routes through this basic (non-MVE-optimized) path (`in_ch <=
REVERSE_TCOL_EFFICIENT_THRESHOLD == 16`), against their official
TensorFlow-generated golden data, comparing the compiled binaries'
output directly (bypassing the Unity harness):
- `transpose_conv_1`: 0/1944 differ
- `transpose_conv_2`: 0/2016 differ
- `transpose_conv_3`: 0/1050 differ (this one already exercises `pad_x %
stride_x != 0`, confirming the horizontal axis is untouched by this fix)
- `reverse_transpose_conv_4`: 0/40 differ
All four already passed before this change and still pass after.
6. **Synthetic fuzz sweep**: 6 additional independently-chosen `(pad,
stride, filter, size, seed)` combinations against the validated
reference. Every case with `pad_y % stride_y == 0` gave 0 mismatches on
both old and new code (identical, as the algebra predicts they must be).
Every case with `pad_y % stride_y != 0` gave massive failure on the old
code (97-100% of elements wrong) and 0 mismatches on the new code. One
case isolated `pad_x % stride_x != 0` with `pad_y % stride_y == 0`: both
old and new gave 0 mismatches, confirming horizontal misalignment was
already handled correctly and is untouched by this fix.
7. **Why this escaped CI**: all 5 existing official test cases that
exercise this function (`transpose_conv_1..4`,
`reverse_transpose_conv_4`) happen to use padding/stride pairs where
`pad_y % stride_y == 0` (2%2, 0%1, 0%5, 0%1, 0%1) — exactly the
condition under which the old and new code are provably identical. That
is why a bug of this shape was never caught by the existing suite; it
isn't a gap in test *execution*, it's a gap in the *parameter space* the
existing fixtures happen to cover.
Two separate, pre-existing issues unrelated to this bug were noticed
during fuzzing (the function crashing when the filter is smaller than
the stride, and when padding exceeds the filter size); both reproduce
identically on unmodified upstream and on this patched branch, so they
are out of scope for this PR and are not addressed here.
## Disclosure
This investigation, the reproduction/verification harnesses, and the fix
implementation were done with Claude AI assistance (root-cause
derivation, host-compilable test harnesses against the real library
source, and the independent reference implementation used as the
correctness oracle). I reviewed the diagnosis, the fix, and all
verification results myself before submitting this PR, and I'm happy to
answer questions or make changes based on maintainer feedback.
---------
Signed-off-by: 94xhn <87560781+94xhn@users.noreply.github.com>1 parent 8100ecb commit 4a8f5b5
1 file changed
Lines changed: 13 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
21 | 21 | | |
22 | 22 | | |
23 | 23 | | |
24 | | - | |
25 | | - | |
| 24 | + | |
| 25 | + | |
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
| |||
138 | 138 | | |
139 | 139 | | |
140 | 140 | | |
141 | | - | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
142 | 151 | | |
143 | | - | |
| 152 | + | |
144 | 153 | | |
145 | 154 | | |
146 | 155 | | |
| |||
0 commit comments