Skip to content

[MPS] Extend addmm to integral types #160270

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

malfet
Copy link
Contributor

@malfet malfet commented Aug 10, 2025

Stack from ghstack (oldest at bottom):

By adding addmm kernel, which is a logical continuation of mm one. The only tricking part are how alpha and beta constants are handled, which are passed as optmath_t, i.e. that it could be, int64, int32 or float

Unified all MM flavors instantiations thru INSTANTIATE_MM_OPS and tested that addmm metal kernel works as expected for floating types as well by testing it via

 PYTORCH_MPS_PREFER_METAL=1 python test/test_mps.py -v -k test_output_match_addmm_mps_

Fixes #154901

@malfet malfet requested a review from kulinseth as a code owner August 10, 2025 12:16
Copy link

pytorch-bot bot commented Aug 10, 2025

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/160270

Note: Links to docs will display an error until the docs builds have been completed.

⏳ No Failures, 22 Pending

As of commit cdfaf5a with merge base 86eb65f (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

malfet added a commit that referenced this pull request Aug 10, 2025
Fixes #154901

ghstack-source-id: 70c05c2
Pull Request resolved: #160270
@pytorch-bot pytorch-bot bot added ciflow/mps Run MPS tests (subset of trunk) release notes: mps Release notes category labels Aug 10, 2025
@malfet malfet added the topic: bug fixes topic category label Aug 10, 2025
}
});
return output;
return output;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Double return? Surprised this didn't error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I hoped some of the linters will be triggered by it, but feels like this is fine...

constant uint3& sizes [[buffer(6)]],
uint2 tid [[thread_position_in_threadgroup]],
uint2 thread_id [[thread_position_in_grid]]) {
threadgroup T A_tile[TILE_DIM][TILE_DIM];
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's ugly, but can this be rewritten as an std array too?

Copy link
Contributor Author

@malfet malfet Aug 10, 2025

Choose a reason for hiding this comment

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

Threadgroups are a bit weird(i.e. this statement affects GPU occupancy), let me give it a try in a separate PR, but make sure it would not regress the perf...

malfet added a commit that referenced this pull request Aug 10, 2025
Fixes #154901

ghstack-source-id: c2c210e
Pull Request resolved: #160270
@malfet
Copy link
Contributor Author

malfet commented Aug 11, 2025

@pytorchbot merge -f "Lint + MPS are green"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/mps Run MPS tests (subset of trunk) Merged release notes: mps Release notes category topic: bug fixes topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants