Skip to content
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

Enable nvprims.transpose fusions for nvFuser #86967

Closed

Conversation

IvanYashchuk
Copy link
Collaborator

@IvanYashchuk IvanYashchuk commented Oct 14, 2022

This PR allows transposes to be fused with other operations. If a fusion group is formed only from operations that just manipulate metadata in PyTorch (transpose, view, etc.) then this group is not sent to nvFuser.
On top of that if we have converted to nvprims but then decided to not form a fusion group we modify the graph use prim.impl_aten attribute instead of calling prim(*args, **kwargs) that has a higher overhead.

cc @kevinstephano @jjsjann123

@pytorch-bot pytorch-bot bot added the release notes: fx release notes category label Oct 14, 2022
@pytorch-bot
Copy link

pytorch-bot bot commented Oct 14, 2022

🔗 Helpful Links

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

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

✅ No Failures

As of commit b013d6f:
💚 Looks good so far! There are no failures yet. 💚

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

@IvanYashchuk
Copy link
Collaborator Author

@SherlockNoMad could you please review the torch/fx/passes/infra/partitioner.py changes?

@IvanYashchuk IvanYashchuk changed the title Enable transpose fusions for nvFuser Enable nvprims.transpose fusions for nvFuser Oct 14, 2022
jjsjann123 added a commit to csarofeen/pytorch that referenced this pull request Oct 14, 2022
```
commit 5863911
(ivan/nvprims-transpose-partitioner)
Merge: 73669a7 fc3afc8
Author: Ivan Yashchuk <IvanYashchuk@users.noreply.github.com>
Date:   Fri Oct 14 22:58:55 2022 +0300

    Merge branch 'master' into nvprims-transpose-partitioner
```
@IvanYashchuk
Copy link
Collaborator Author

@ngimel could you please take a look? It's purely nvFuser related change and doesn't modify anything else.

Copy link
Collaborator

@jjsjann123 jjsjann123 left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -268,6 +268,29 @@ def __call__(self, *args):
)


# A set of operators that are supported by nvFuser
# but should not form a fusion group solely on their own
# _non_compute_ops = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: commented code should be removed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's an example, but it should have been marked so more clearly 😄

# "torch.ops.nvprims.broadcast_in_dim.default",
# "torch.ops.nvprims.squeeze.default",
# }
_non_compute_ops = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: peeking through op return type and categorizing view-like return to non-compute ops is counter-intuitive. (thinking about in-place update).

I think this is safe for our uses, but should we rename this to _view_like_ops and add a line in the comment that view-like ops are likely no-compute ops, since in-place update is handled by functionalization

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with you, I used the same name as was already used in the partitioner code.
We also don't have the in-place update (not merged yet).

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 17, 2022
@IvanYashchuk
Copy link
Collaborator Author

@SherlockNoMad could you please review the torch/fx/passes/infra/partitioner.py changes?

@SherlockNoMad

Copy link
Contributor

@SherlockNoMad SherlockNoMad left a comment

Choose a reason for hiding this comment

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

partitioner change looks good to me.

@IvanYashchuk IvanYashchuk added the topic: not user facing topic category label Oct 26, 2022
@IvanYashchuk
Copy link
Collaborator Author

@pytorchbot merge -g

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (ETA: 0-4 Hours).

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

@IvanYashchuk IvanYashchuk deleted the nvprims-transpose-partitioner branch October 26, 2022 18:47
sgrigory pushed a commit to sgrigory/pytorch that referenced this pull request Oct 28, 2022
This PR allows transposes to be fused with other operations. If a fusion group is formed only from operations that just manipulate metadata in PyTorch (transpose, view, etc.) then this group is not sent to nvFuser.
On top of that if we have converted to `nvprims` but then decided to not form a fusion group we modify the graph use `prim.impl_aten` attribute instead of calling `prim(*args, **kwargs)` that has a higher overhead.

cc @kevinstephano @jjsjann123
Pull Request resolved: pytorch#86967
Approved by: https://github.com/jjsjann123, https://github.com/SherlockNoMad
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Nov 5, 2022
This PR allows transposes to be fused with other operations. If a fusion group is formed only from operations that just manipulate metadata in PyTorch (transpose, view, etc.) then this group is not sent to nvFuser.
On top of that if we have converted to `nvprims` but then decided to not form a fusion group we modify the graph use `prim.impl_aten` attribute instead of calling `prim(*args, **kwargs)` that has a higher overhead.

cc @kevinstephano @jjsjann123
Pull Request resolved: pytorch#86967
Approved by: https://github.com/jjsjann123, https://github.com/SherlockNoMad
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Dec 10, 2022
This PR allows transposes to be fused with other operations. If a fusion group is formed only from operations that just manipulate metadata in PyTorch (transpose, view, etc.) then this group is not sent to nvFuser.
On top of that if we have converted to `nvprims` but then decided to not form a fusion group we modify the graph use `prim.impl_aten` attribute instead of calling `prim(*args, **kwargs)` that has a higher overhead.

cc @kevinstephano @jjsjann123
Pull Request resolved: pytorch#86967
Approved by: https://github.com/jjsjann123, https://github.com/SherlockNoMad
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged module: nvfuser open source release notes: fx release notes category topic: not user facing topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants