Zero grad_input in max_pool2d_with_indices_backward - #21687
Conversation
max_pool_backward_impl scatter-adds into grad_input, writing only the argmax positions. The kernel only resizes grad_input and never clears it, and the memory planner recycles arena buffers across ops and iterations, so every non-argmax element accumulates onto stale data. ATen performs the same accumulation but zeroes first, in max_pool2d_with_indices_backward_out_cpu (aten/src/ATen/native/DilatedMaxPool2d.cpp); aten/src/ATen/native/cpu/MaxPoolKernel.cpp holds the identical += loop. The loop was ported here without the zeroing. Any trainable graph containing Conv2d -> MaxPool2d therefore gets corrupted weight gradients: a 67k-param CNN explodes to NaN within 3 steps, while an 11.2M-param ResNet-18 produces no NaN at all and silently fails to converge. Measured with identical flags and a fixed batch, only this file differing: Conv2d->MaxPool2d->Linear goes from 2.309544 -> NaN (291/300 NaN steps) to 2.309544 -> 0.003952 (0 NaN), and five other trainable models (strided-conv, conv-only, pool-only, MLP) produce byte-identical loss curves. Confirmed on macOS arm64 and on a Snapdragon 845 handset; step latency cost is +0.06%. Fixes pytorch#21686
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21687
Note: Links to docs will display an error until the docs builds have been completed.
|
|
|
|
Hi @anurag2796! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect gradient computation in the portable CPU implementation of max_pool2d_with_indices_backward by ensuring grad_input is zero-initialized before the kernel’s scatter-add loop runs. This aligns ExecuTorch behavior with ATen’s implementation and prevents stale arena-buffer contents from corrupting downstream gradients (notably convolution_backward) in trainable graphs.
Changes:
- Add
<cstring>include for byte-level initialization utilities. - Zero
grad_inputviamemset(..., 0, grad_input.nbytes())afterresize_tensorand beforemax_pool_backward_implperforms+=scatter-add.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Fixes #21686.
max_pool_backward_implscatter-adds intograd_input, writing only the argmax positions:but the kernel only resizes
grad_inputand never clears it. Since the memory planner recycles arenabuffers across ops and across iterations, every non-argmax element accumulates onto stale data.
ATen does the same accumulation and zeroes first —
DilatedMaxPool2d.cppcalls
gradInput.zero_()before dispatching, andMaxPoolKernel.cppholds the character-for-character same
+=loop. The loop was ported here; thezero_()was not.Effect
Any trainable graph containing
Conv2d -> MaxPool2dgets corrupted weight gradients, because thepolluted
grad_inputfeedsconvolution_backward. Pool-only graphs are unaffected — there the maxpoolgrad_inputis the gradient w.r.t. the network input and is discarded before reaching a parameter,which is why the bug looks so selective.
The failure mode depends on model size, and the large-model case is the concerning one:
Conv2d->MaxPool2d->Linear(67,642 params)At 11M parameters there is no NaN at all — the loss just wanders and ends above where it started.
Validation
strided variant) produce byte-identical loss curves before and after, under identical flags and a
fixed batch, with only this file changed.
patched curve is bit-identical for the first 5 steps (max abs deviation
0.000e+00) and trackswithin <1% from step 100 to 300.
arm64-v8a(NDK 27.1.12297006), run on a Snapdragon 845 / Android 10handset pinned to the big cluster. Same before/after, and the fixed curve agrees with the macOS arm64
host to
3.07e-04max deviation, within 1 ULP on the final loss.All-zero is the correct bit pattern for every dtype
ET_SWITCH_FLOATHBF16_TYPEScovers(float / half / bfloat16).
Reproduced on v1.3.1 and verified still present on
mainand in v1.4.0.