Skip to content

Replace split-squeeze pattern #101765

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 11 commits into from
Closed

Replace split-squeeze pattern #101765

wants to merge 11 commits into from

Conversation

@devashishshankar
Copy link
Contributor

@devashishshankar devashishshankar commented May 18, 2023 •
edited by pytorch-bot bot
Loading

Stack from ghstack (oldest at bottom):

Replaces split-squeeze (same dimension) with an unbind. This will be used in combination with later patterns to remove the unbind if it follows a cat/stack

Differential Revision: D45758181

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @Xia-Weiwen @wenzhe-nrv @jiayisunx @peterbell10 @desertfire

@devashishshankar
Replace split-squeeze pattern
996cc1d 
Replaces split-squeeze (same dimension) with an unbind. This will be used in combination with later patterns to remove the unbind if it follows a cat/stack

Differential Revision: [D45758181](https://our.internmc.facebook.com/intern/diff/D45758181/)

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented May 18, 2023 •
edited
Loading

🔗 Helpful Links

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

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

✅ No Failures

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

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

This was referenced May 18, 2023
Closed
Closed
Closed
@devashishshankar devashishshankar requested a review from jansel May 18, 2023 03:22
@devashishshankar
Update on "Replace split-squeeze pattern"
20f45b6 
Replaces split-squeeze (same dimension) with an unbind. This will be used in combination with later patterns to remove the unbind if it follows a cat/stack

Differential Revision: [D45758181](https://our.internmc.facebook.com/intern/diff/D45758181/)

[ghstack-poisoned]
@devashishshankar
Update on "Replace split-squeeze pattern"
756016f 
Replaces split-squeeze (same dimension) with an unbind. This will be used in combination with later patterns to remove the unbind if it follows a cat/stack

Differential Revision: [D45758181](https://our.internmc.facebook.com/intern/diff/D45758181/)

[ghstack-poisoned]
@devashishshankar
Copy link
Contributor Author

@pytorchbot label "topic: not user facing"

@pytorch-bot pytorch-bot bot added the topic: not user facing topic category label May 18, 2023
@devashishshankar
Update on "Replace split-squeeze pattern"
08f8de7 
Replaces split-squeeze (same dimension) with an unbind. This will be used in combination with later patterns to remove the unbind if it follows a cat/stack

Differential Revision: [D45758181](https://our.internmc.facebook.com/intern/diff/D45758181/)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
jansel
jansel reviewed May 18, 2023
View reviewed changes
torch/_inductor/fx_passes/split_cat.py Outdated
continue
for node in other_node.users:
if node not in searched:
if not matching_nodes or self._match_fns(node):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain what matching_nodes does?

Copy link
Contributor Author

@devashishshankar devashishshankar May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So, I added this in the base class (_TargetArgsExpr) - and it is here just to follow the interface.

By default, we only returned nodes which match in find_anchor_nodes. This doesn't work for RepeatedExpr. In the usual case, we want to not match the pattern, unless all users of the previous node match it. With the usual behavior, it would by default do just a partial match.

So things like

split -> squeeze
      -> relu

will get matched for a split->squeeze pattern.

If I pass, matching_nodes=False, all users of previous node are returned, allowing me to invalidate partial matches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you have a better way of doing this please suggest. I was initially thinking of using a different method like find_sibling_nodes, but it seemed very similar to find_anchor_nodes in implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

btw, I just realized I forgot to add this variable to some other classes. I can add them, but waiting for your response first.

Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't really what find_anchor_nodes() is meant for, I worry this confuses the semantics.

I'd rather see an extra check, either in the _match function or in the handler that is just based on the users of split.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do that

@devashishshankar
Update on "Replace split-squeeze pattern"
4fbccf8 
Replaces split-squeeze (same dimension) with an unbind. This will be used in combination with later patterns to remove the unbind if it follows a cat/stack

Differential Revision: [D45758181](https://our.internmc.facebook.com/intern/diff/D45758181/)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@devashishshankar
Update on "Replace split-squeeze pattern"
b76a9e0 
Replaces split-squeeze (same dimension) with an unbind. This will be used in combination with later patterns to remove the unbind if it follows a cat/stack

Differential Revision: [D45758181](https://our.internmc.facebook.com/intern/diff/D45758181/)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@devashishshankar
Update on "Replace split-squeeze pattern"
38fc561 
Replaces split-squeeze (same dimension) with an unbind. This will be used in combination with later patterns to remove the unbind if it follows a cat/stack

Differential Revision: [D45758181](https://our.internmc.facebook.com/intern/diff/D45758181/)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@devashishshankar
Update on "Replace split-squeeze pattern"
7532fda 
Replaces split-squeeze (same dimension) with an unbind. This will be used in combination with later patterns to remove the unbind if it follows a cat/stack

Differential Revision: [D45758181](https://our.internmc.facebook.com/intern/diff/D45758181/)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@devashishshankar
Update on "Replace split-squeeze pattern"
6491aba 
Replaces split-squeeze (same dimension) with an unbind. This will be used in combination with later patterns to remove the unbind if it follows a cat/stack

Differential Revision: [D45758181](https://our.internmc.facebook.com/intern/diff/D45758181/)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@devashishshankar
Update on "Replace split-squeeze pattern"
f8425f7 
Replaces split-squeeze (same dimension) with an unbind. This will be used in combination with later patterns to remove the unbind if it follows a cat/stack

Differential Revision: [D45758181](https://our.internmc.facebook.com/intern/diff/D45758181/)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@devashishshankar
Update on "Replace split-squeeze pattern"
59ee628 
Replaces split-squeeze (same dimension) with an unbind. This will be used in combination with later patterns to remove the unbind if it follows a cat/stack

Differential Revision: [D45758181](https://our.internmc.facebook.com/intern/diff/D45758181/)

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng Xia-Weiwen wenzhe-nrv jiayisunx peterbell10 desertfire

[ghstack-poisoned]
@facebook-github-bot
Copy link
Contributor

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label May 24, 2023
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (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

@facebook-github-bot facebook-github-bot deleted the gh/devashishshankar/8/head branch June 8, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jansel jansel jansel approved these changes

Assignees

No one assigned

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: inductor topic: not user facing topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@devashishshankar @facebook-github-bot @pytorchmergebot @jansel