vulkan: optimize operations in the IM2COL shader#22685
Open
daniandtheweb wants to merge 2 commits intoggml-org:masterfrom
Open
vulkan: optimize operations in the IM2COL shader#22685daniandtheweb wants to merge 2 commits intoggml-org:masterfrom
daniandtheweb wants to merge 2 commits intoggml-org:masterfrom
Conversation
Contributor
|
Perf on RTX 5090: |
jeffbolznv
reviewed
May 4, 2026
| const uint delta_ic = BLOCK_SIZE / KHKW; | ||
| const uint delta_rem = BLOCK_SIZE % KHKW; | ||
| const uint delta_ky = delta_rem / p.KW; | ||
| const uint delta_kx = delta_rem % p.KW; |
Contributor
There was a problem hiding this comment.
I'm not totally following this. In general it seems unsafe to precompute divs/mods and add them, as sometimes you would wrap to the next value and need to do a fixup. Maybe that's what the fixup logic is doing, but it's not clear.
I wonder if it might be better to pass KW as a spec constant and let the compiler transform it into something faster.
Contributor
Author
There was a problem hiding this comment.
kx_wrap and ky_wrap should take care of the wrapping. Moreover it shouldn't be possible for the values to wrap around twice as:
delta_kxby using a modulo is always less thanp.KW,kxis always less thanp.KWbecause of the modulo and the maximum value that it can reach is2 * p.KW - 2which is always less than2 * p.KW.delta_ky's max value isp.KH - 1so always less thanp.KH,kyis at mostp.KH - 1and the maximum value that it can reach is2 * p.KH - 1so less than2 * p.KH.
Contributor
Author
There was a problem hiding this comment.
I'm not that experienced on vulkan shaders so the most I could confidently achieve was this as it should (in theory) be mathematically correct. If there are better approaches I'll gladly look into them.
Contributor
Author
There was a problem hiding this comment.
Maybe it's better if I add some comments on the wrap values to make it more clear?
Contributor
There was a problem hiding this comment.
Comments would help, but I think spec constants would keep the code more clear. I'll defer to @0cc4m on what to do.
Contributor
Author
There was a problem hiding this comment.
Got it, I'll start by adding some comments on the most confusing parts for now.
github-actions
Bot
added
Vulkan
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
2 participants
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.
Overview
This optimizes the IM2COL shader by extracting redundant operations from the loops, similar to how I already did it in this: #11826.
Radeon RX 7800XTRadeon RX 5700XTRequirements