[JIT] canonicalize aten::rsub #65014
Merged
Conversation
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
Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/) [ghstack-poisoned]
Contributor
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 1ee85d2 (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions to the (internal) Dr. CI Users group. |
facebook-github-bot
added
oncall: jit
mcr229
pushed a commit
that referenced
this pull request
Sep 14, 2021
Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/) ghstack-source-id: 138056629 Pull Request resolved: #65014
eellison
reviewed
Sep 14, 2021
Contributor
eellison
left a comment
eellison
left a comment
There was a problem hiding this comment.
Looks good!! a few comments from me. test ?
| // Normalize rsub such that `rsub(x,y) = sub(x,y)` | ||
| bool normalizeRSub(graph_node_list_iterator& iter) { | ||
| ArrayRef<Value*> args = iter->inputs(); | ||
| if (iter->kind() == aten::rsub) { |
Contributor
There was a problem hiding this comment.
we all overloads for any particular symbol, so it's possible unlikely that there are other aten::rsub operators with different typed inputs for which this substitution isnt valid. just to be safe and future proof we should check the full schema here. you can use the node->matches("rsub.Tensor(Tensor self, Tensor other, *, Scalar alpha=1) -> Tensor") api.
See https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/native/native_functions.yaml#L4880, for this operator what we're doing isnt valid
|
|
||
| // Normalize rsub such that `rsub(x,y) = sub(x,y)` | ||
| bool normalizeRSub(graph_node_list_iterator& iter) { | ||
| ArrayRef<Value*> args = iter->inputs(); |
Contributor
There was a problem hiding this comment.
nit: move args into the if block
| bool normalizeRSub(graph_node_list_iterator& iter) { | ||
| ArrayRef<Value*> args = iter->inputs(); | ||
| if (iter->kind() == aten::rsub) { | ||
| iter->replaceWithNewSymbol(aten::sub); |
Contributor
There was a problem hiding this comment.
replaceWithNewSymbol returns the updated node, so the replacements we're doing in the following lines will not affect it but the existing node we're destroying
Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/) [ghstack-poisoned]
mcr229
pushed a commit
that referenced
this pull request
Sep 15, 2021
Pull Request resolved: #65014 ghstack-source-id: 138181235 Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/)
Contributor
Author
|
I tried running the test through buck: |
Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/) [ghstack-poisoned]
mcr229
pushed a commit
that referenced
this pull request
Sep 16, 2021
Pull Request resolved: #65014 ghstack-source-id: 138221870 Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/)
Contributor
Author
|
Ran tests |
eellison
reviewed
Sep 16, 2021
Contributor
eellison
left a comment
eellison
left a comment
There was a problem hiding this comment.
Nice! just one small comment
test/jit/test_peephole.py
Outdated
| FileCheck().check("conv").check("dim").run(conv_dim.graph) | ||
|
|
||
| def test_normalized_rsub(self): | ||
| def convertible_rsub(x: int, y: int): |
Contributor
There was a problem hiding this comment.
i think we should be checking tensor inputs here
Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/) [ghstack-poisoned]
Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/) [ghstack-poisoned]
mcr229
pushed a commit
that referenced
this pull request
Sep 16, 2021
Pull Request resolved: #65014 ghstack-source-id: 138262596 Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/)
Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/) [ghstack-poisoned]
mcr229
pushed a commit
that referenced
this pull request
Sep 17, 2021
Pull Request resolved: #65014 ghstack-source-id: 138331729 Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/)
mcr229
commented
Sep 17, 2021
| {aten::true_divide_, aten::div_}, | ||
| {aten::concat, aten::cat}, | ||
| {aten::row_stack, aten::vstack}, | ||
| {aten::rsub, aten::sub}, |
Contributor
Author
There was a problem hiding this comment.
the fix for this test
seemed to be that rsub could not be found within this map, so adding it to the map and making the appropriate changes seemed to make the test pass
Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/) [ghstack-poisoned]
mcr229
pushed a commit
that referenced
this pull request
Sep 17, 2021
Pull Request resolved: #65014 ghstack-source-id: 138386559 Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/)
Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/) [ghstack-poisoned]
Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/) [ghstack-poisoned]
mcr229
pushed a commit
that referenced
this pull request
Sep 21, 2021
Pull Request resolved: #65014 ghstack-source-id: 138562901 Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/)
eellison
approved these changes
Sep 21, 2021
| if dtype == torch.float32: | ||
| # TODO: no reason why we cant run this with tracing graph | ||
| if support_script: | ||
| if support_script and op.name != "rsub": |
Contributor
There was a problem hiding this comment.
@alanwaketan this is an example of an annoying test quirk that your PR will fix
mcr229
pushed a commit
that referenced
this pull request
Sep 21, 2021
Pull Request resolved: #65014 ghstack-source-id: 138656948 Differential Revision: [D30941389](https://our.internmc.facebook.com/intern/diff/D30941389/)
facebook-github-bot
pushed a commit
that referenced
this pull request
Sep 23, 2021
Summary: Pull Request resolved: #65014 ghstack-source-id: 138656948 Test Plan: ``` (pytorch) [[email protected] ~/pytorch] python3 test/test_jit.py TestPeephole CUDA not available, skipping tests monkeytype is not installed. Skipping tests for Profile-Directed Typing ........s...................... ---------------------------------------------------------------------- Ran 31 tests in 0.393s OK (skipped=1) (pytorch) [[email protected] ~/pytorch] python3 test/test_jit.py TestPeephole.test_normalized_rsub CUDA not available, skipping tests monkeytype is not installed. Skipping tests for Profile-Directed Typing . ---------------------------------------------------------------------- Ran 1 test in 0.015s OK ``` Reviewed By: eellison Differential Revision: D30941389 fbshipit-source-id: 03f0416d99090845c9bfb1e5fcf771d5f1d7a050
khabinov
added a commit
to khabinov/glow
that referenced
this pull request
Sep 23, 2021
Summary: After a recent optimization pytorch/pytorch#65014 we started to normalize `aten::rsub` as `aten::sub`, so the test started to fail. This diff makes sure the test is passing. Reviewed By: wushirong Differential Revision: D31153947 fbshipit-source-id: 3ea52f8b7b99d164084390548be22a82e9f09c8c
khabinov
added a commit
to khabinov/glow
that referenced
this pull request
Sep 23, 2021
Summary: Pull Request resolved: pytorch#5824 After a recent optimization pytorch/pytorch#65014 we started to normalize `aten::rsub` as `aten::sub`, so the test started to fail. This diff makes sure the test is passing. Reviewed By: wushirong Differential Revision: D31153947 fbshipit-source-id: b2a299274e5c440cd52342087cfcdb0b387cc4fb
khabinov
added a commit
to khabinov/glow
that referenced
this pull request
Sep 23, 2021
Summary: Pull Request resolved: pytorch#5824 After a recent optimization pytorch/pytorch#65014 we started to normalize `aten::rsub` as `aten::sub`, so the test started to fail. This diff makes sure the test is passing. Reviewed By: wushirong Differential Revision: D31153947 fbshipit-source-id: d7034b18c2e44f4ce76481ed68e7ebf94c1bcfa1
facebook-github-bot
pushed a commit
to pytorch/glow
that referenced
this pull request
Sep 23, 2021
Summary: Pull Request resolved: #5824 After a recent optimization pytorch/pytorch#65014 we started to normalize `aten::rsub` as `aten::sub`, so the test started to fail. This diff makes sure the test is passing. Reviewed By: wushirong Differential Revision: D31153947 fbshipit-source-id: 2aab03647a9910fddeb974a6d451ce3fa5376e4b
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Stack from ghstack:
Differential Revision: D30941389