ag_gemm bench: restore num_comm_sms for shapes absent from SMS_PER_SHAPE - #36
Open
monopodium wants to merge 1 commit into
Open
ag_gemm bench: restore num_comm_sms for shapes absent from SMS_PER_SHAPE#36monopodium wants to merge 1 commit into
monopodium wants to merge 1 commit into
Conversation
The per-shape override assigns into args inside the shape loop and never restores it, so any shape not in SMS_PER_SHAPE silently inherits whatever the previous shape set rather than the CLI value. Sweeping 4096,8192,16384,32768 leaves 32768 running with 8 comm CTAs instead of the 64 default, because 32768 is not in the table and 16384 is. The adjacent INTRA_OVERRIDE block already guards against exactly this with an else that restores cli_num_intra_comm_sms - added, per the comment above it, after the same class of bug was found there. This mirrors that. Measured impact where it could be measured: at M=32768 the shape runs 8.596 ms on its own and 10.636 ms when preceded by another shape in the same sweep, so any multi-shape number for a table-absent shape is off by about 24%.
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
1 participant
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.
The per-shape override assigns into args inside the shape loop and never restores it, so any shape not in SMS_PER_SHAPE silently inherits whatever the previous shape set rather than the CLI value. Sweeping 4096,8192,16384,32768 leaves 32768 running with 8 comm CTAs instead of the 64 default, because 32768 is not in the table and 16384 is.
The adjacent INTRA_OVERRIDE block already guards against exactly this with an else that restores cli_num_intra_comm_sms - added, per the comment above it, after the same class of bug was found there. This mirrors that.
Measured impact where it could be measured: at M=32768 the shape runs 8.596 ms on its own and 10.636 ms when preceded by another shape in the same sweep, so any multi-shape number for a table-absent shape is off by about 24%.