Skip to content

Fix conv2d OOB reads for partial output-channel tiles - #4653

Merged
zcbenz merged 3 commits into
ml-explore:mainfrom
navthings:main
Oct 16, 2026
Merged

zcbenz merged 3 commits into
ml-explore:mainfrom
navthings:main

Conversation

@navthings

@navthings navthings commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor
  • ☑️ I understand it is strictly prohibited to use AI to write PR description
  • AI usage disclosure: didn't use ai tools for this one, quite a straight forward fix

Fixes #4630.

partial output-channel tiles could take the fast path and read past the end of the weights buffer.
removed the BN != 8 shortcut from the 2D, 3D, and small-channel loaders so partial tiles use the safe load path.

Tested the issue repro with Metal shader validation on:

  • M=9601, O=32
  • M=9601, O=48
  • M=9601, O=96
  • M=8192, O=32

Added regression tests for partial output-channel tiles.

Tested on an M5 MacBook Air running macOS Golden Gate 27.

The 54 conv test failures also occur on main at 3c40e8f without this change.

@zcbenz zcbenz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix, I'm good with the changes but would need a second view from @jagrit06

@zcbenz
zcbenz requested a review from jagrit06 October 9, 2026 07:08
@navthings

Copy link
Copy Markdown
Contributor Author

cheers! and thats fine.

@navthings

Copy link
Copy Markdown
Contributor Author

will this pr be looked at soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Metal] implicit-GEMM conv2d reads past the weight buffer when the output channels are not a multiple of the tile width

2 participants