[Kernels] Fix static conv width-padding boundaries - #6788
#6788Open
blinding-pixels wants to merge 2 commits into
modular:mainmodular/modular:mainfrom
blinding-pixels:agent/fix-static-conv-width-paddingblinding-pixels/modular:agent/fix-static-conv-width-paddingCopy head branch name to clipboard
Open
[Kernels] Fix static conv width-padding boundaries#6788blinding-pixels wants to merge 2 commits intomodular:mainmodular/modular:mainfrom blinding-pixels:agent/fix-static-conv-width-paddingblinding-pixels/modular:agent/fix-static-conv-width-paddingCopy head branch name to clipboard
blinding-pixels wants to merge 2 commits into
modular:mainmodular/modular:mainfrom
blinding-pixels:agent/fix-static-conv-width-paddingblinding-pixels/modular:agent/fix-static-conv-width-paddingCopy head branch name to clipboard
Conversation
BEGIN_PUBLIC [Kernels] Fix static conv width-padding boundaries Derive tile padding boundaries from convolution geometry. Assisted-by: AI END_PUBLIC Signed-off-by: blinding-pixels <281499151+blinding-pixels@users.noreply.github.com>
Signed-off-by: blinding-pixels <281499151+blinding-pixels@users.noreply.github.com>
|
All contributors have signed the CLA ✍️ ✅ |
Author
|
I have read the CLA Document and I hereby sign the CLA |
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.
Linked issue
Fixes #6712
(#6712 is currently unassigned and
Needs Triage— no maintainer review yet. Opening this to surface the concrete fix; happy to hold for direction on the approach.)Type of change
Motivation
The static conv padding path miscomputes outputs near the padding boundary. At
wo=16(K=8) it returns 23 where the correct value is 21, because the static path doesn't derive per-tile padding adjustments the way the non-static path does. (I picked this up as an entry point into the conv kernels — small and self-contained.)What changed
Compute the left and right padding-impact boundaries from the convolution's padding, stride, dilation, input width, and kernel width — the same geometry the non-static path already uses. The static path keeps its fixed-width micro-kernel tiles, but now: marks every tile that has a true padding boundary, passes the tile's output-width offset into
_inner_loops_static, and computes each tap's padding adjustment relative to that tile offset. Net −71 lines inconv.mojo(37 added, 108 removed).Testing
Added a deterministic 16-case regression sweep. All-ones inputs with packed filters give an analytical oracle: each output must equal the exact number of valid taps times the channel count, which checks every output element and inherently rejects NaNs. The fixed kernel passes all 16 cases exactly. As a negative control, the same test fails against the original implementation in 3/3 uncached runs at the K=8 boundary (
wo=16, expected 21, actual 23).Checklist
./bazelw run formatto format my changesAssisted-by:trailerAssisted-by: AI