Skip to content

Add metal kernel for log ops #153398

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

Add metal kernel for log ops #153398

wants to merge 5 commits into from

Conversation

skotapati
Copy link
Collaborator

@skotapati skotapati commented May 12, 2025 •
edited
Loading

Move unary log ops to metal kernels

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented May 12, 2025 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

As of commit 9293a46 with merge base 7482eb2 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@pytorch-bot pytorch-bot bot added ciflow/mps Run MPS tests (subset of trunk) release notes: mps Release notes category labels May 12, 2025
@github-actions GitHub Actions
Copy link
Contributor

Attention! native_functions.yaml was changed

If you are adding a new function or defaulted argument to native_functions.yaml, you cannot use it from pre-existing Python frontend code until our FC window passes (two weeks). Split your PR into two PRs, one which adds the new C++ functionality, and one that makes use of it from Python, and land them two weeks apart. See https://github.com/pytorch/pytorch/wiki/PyTorch's-Python-Frontend-Backward-and-Forward-Compatibility-Policy#forwards-compatibility-fc for more info.

Caused by:

@skotapati
Copy link
Collaborator Author

@pytorchbot drci

@skotapati skotapati marked this pull request as ready for review May 15, 2025 17:19
@skotapati
Copy link
Collaborator Author

@pytorchbot rebase

@pytorchmergebot
Copy link
Collaborator

@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here

@pytorchmergebot
Copy link
Collaborator

Successfully rebased dev/skotapati/unary_log_ops onto refs/remotes/origin/viable/strict, please pull locally before adding more changes (for example, via git checkout dev/skotapati/unary_log_ops && git pull --rebase)

@pytorchmergebot pytorchmergebot force-pushed the dev/skotapati/unary_log_ops branch from 7fb387d to 9293a46 Compare May 15, 2025 18:13
malfet
malfet requested changes May 16, 2025
View reviewed changes
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Why do you need MPS fallback for MacOS-13? Just dispatch to metal on all versions of OS

aten/src/ATen/native/mps/operations/UnaryOps.mm
#define CREATE_MPS_FALLBACK_UNARY_TORCH_IMPL_FUNC(func_out, func_stub, func_name) \
TORCH_IMPL_FUNC(func_out)(const Tensor& self, const Tensor& output) { \
static const bool is_macOS_14_0_or_newer = is_macos_13_or_newer(MacOSVersion::MACOS_VER_14_0_PLUS); \
if (is_macOS_14_0_or_newer) { \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this check is needed? Wouldn't it be easier to just dispatch to op unconditionally on all versions of MacOS?

@malfet malfet added the topic: improvements topic category label May 16, 2025
malfet
malfet approved these changes May 16, 2025
View reviewed changes
Copy link
Contributor

@malfet malfet left a comment

Choose a reason for hiding this comment

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

Ok, let's land it as is, and I'll do the cleanup in the followup change

@malfet
Copy link
Contributor

malfet commented May 16, 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

@malfet malfet mentioned this pull request May 16, 2025
Closed
pytorchmergebot pushed a commit that referenced this pull request May 16, 2025
@malfet @pytorchmergebot
[BE][MPS] Cleanup log ops migration (#153727)
62d8e3c 
Introduced by #153398

Workaround internal compiler error on MacOS-13 by providing boolean specialization

Pull Request resolved: #153727
Approved by: https://github.com/Skylion007
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kulinseth kulinseth kulinseth approved these changes

@malfet malfet malfet approved these changes

Assignees

No one assigned

Labels

ciflow/mps Run MPS tests (subset of trunk) Merged open source release notes: mps Release notes category topic: improvements topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@skotapati @pytorchmergebot @malfet @kulinseth @pytorchbot