[export] make with_effect mark op has_effect to prevent them from DCEed. #129680
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
[ghstack-poisoned]
ydwu4
requested review from
avikchaudhuri,
gmagogsfm,
tugsbayasgalan and
zhxchen17
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/129680
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ✅ No FailuresAs of commit 498df79 with merge base 5ceba6a ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
angelayi
approved these changes
Jun 27, 2024
…ct_token" Before the PR, custom ops that doesn't return outputs will gets eliminated after calling `.module()` because the effect_token that keeps the operator not DCEed is removed in remove_effect_token. The reason why we want to remove_effect_token is because we don't want the token to be part of input. However, the causes DCE calls in remove_effect_token itself and the dce calls in unlift to remove the custom op in the graph causing an error in the exported graph. This PR removes the DCE calls in unlift and remove_effect_token to avoid this, which may not be a good idea. The idea behind is that the passes in export should never call eliminate dead code so that we could put remote_effect_token anywhere we want. The alternative is that we should carefully put remove_effect_token as the last transformation to avoid any transformations to eliminate them. But in the case of unlift, we have to remove_effect_token before running unlift in order to keep the signature match. Test Plan: Add a new test pytest test/export/test_torchbind.py -k test_export_inplace_custom_op [ghstack-poisoned]
…ct_token" Before the PR, custom ops that doesn't return outputs will gets eliminated after calling `.module()` because the effect_token that keeps the operator not DCEed is removed in remove_effect_token. The reason why we want to remove_effect_token is because we don't want the token to be part of input. However, the causes DCE calls in remove_effect_token itself and the dce calls in unlift to remove the custom op in the graph causing an error in the exported graph. This PR removes the DCE calls in unlift and remove_effect_token to avoid this, which may not be a good idea. The idea behind is that the passes in export should never call eliminate dead code so that we could put remote_effect_token anywhere we want. The alternative is that we should carefully put remove_effect_token as the last transformation to avoid any transformations to eliminate them. But in the case of unlift, we have to remove_effect_token before running unlift in order to keep the signature match. Test Plan: Add a new test pytest test/export/test_torchbind.py -k test_export_inplace_custom_op [ghstack-poisoned]
ydwu4
changed the title
angelayi
approved these changes
Jun 27, 2024
ydwu4
changed the title
…event them from DCEed." Before the PR, custom ops that doesn't return outputs will gets eliminated after calling `.module()` because the effect_token that keeps the operator not DCEed is removed in remove_effect_token. The reason why we want to remove_effect_token is because we don't want the token to be part of input. However, the causes DCE calls in remove_effect_token itself and the dce calls in unlift to remove the custom op in the graph causing an error in the exported graph. This PR calls has_side_effect in with_effect to make sure graph.eliminate_dead_code doesn't remove the calls by accident. Test Plan: Add a new test pytest test/export/test_torchbind.py -k test_export_inplace_custom_op [ghstack-poisoned]
|
@pytorchbot merge |
Merge startedYour 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 |
ydwu4
changed the title
|
@pytorchbot revert -m "breaking internal builds, see D59181183" -c ghfirst |
|
@pytorchbot successfully started a revert job. Check the current status here. |
|
@ydwu4 your PR has been successfully reverted. |
pytorchmergebot
added a commit
that referenced
this pull request
Jul 3, 2024
…from DCEed. (#129680)" This reverts commit 4b8a5e0. Reverted #129680 on behalf of https://github.com/kit1980 due to breaking internal builds, see D59181183 ([comment](#129680 (comment)))
|
@ydwu4 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…em from DCEed." Before the PR, custom ops that don't return outputs will get eliminated after calling `.module()` because the effect_token that keeps the operator alive is removed in remove_effect_token pass. The reason why we want to remove_effect_token is because we don't want the token to be part of input. However, this causes DCE calls in remove_effect_token itself and the dce calls in unlift to remove the custom op in the graph causing an error in the exported graph. This PR calls has_side_effect in with_effect to make sure graph.eliminate_dead_code doesn't remove the calls by accident. Test Plan: Add a new test pytest test/export/test_torchbind.py -k test_export_inplace_custom_op Differential Revision: [D59498728](https://our.internmc.facebook.com/intern/diff/D59498728) [ghstack-poisoned]
|
@ydwu4 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
angelayi
approved these changes
Jul 10, 2024
…em from DCEed." Before the PR, custom ops that don't return outputs will get eliminated after calling `.module()` because the effect_token that keeps the operator alive is removed in remove_effect_token pass. The reason why we want to remove_effect_token is because we don't want the token to be part of input. However, this causes DCE calls in remove_effect_token itself and the dce calls in unlift to remove the custom op in the graph causing an error in the exported graph. This PR calls has_side_effect in with_effect to make sure graph.eliminate_dead_code doesn't remove the calls by accident. Test Plan: Add a new test pytest test/export/test_torchbind.py -k test_export_inplace_custom_op Differential Revision: [D59498728](https://our.internmc.facebook.com/intern/diff/D59498728) [ghstack-poisoned]
|
@ydwu4 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…em from DCEed." Before the PR, custom ops that don't return outputs will get eliminated after calling `.module()` because the effect_token that keeps the operator alive is removed in remove_effect_token pass. The reason why we want to remove_effect_token is because we don't want the token to be part of input. However, this causes DCE calls in remove_effect_token itself and the dce calls in unlift to remove the custom op in the graph causing an error in the exported graph. This PR calls has_side_effect in with_effect to make sure graph.eliminate_dead_code doesn't remove the calls by accident. Test Plan: Add a new test pytest test/export/test_torchbind.py -k test_export_inplace_custom_op Differential Revision: [D59498728](https://our.internmc.facebook.com/intern/diff/D59498728) [ghstack-poisoned]
|
@ydwu4 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pytorchbot merge |
Merge failedReason: This PR has internal changes and must be landed via Phabricator Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -f "landed internally" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
xuhancn
pushed a commit
to xuhancn/pytorch
that referenced
this pull request
Jul 25, 2024
…ed. (pytorch#129680) Before the PR, custom ops that don't return outputs will get eliminated after calling `.module()` because the effect_token that keeps the operator alive is removed in remove_effect_token pass. The reason why we want to remove_effect_token is because we don't want the token to be part of input. However, this causes DCE calls in remove_effect_token itself and the dce calls in unlift to remove the custom op in the graph causing an error in the exported graph. This PR calls has_side_effect in with_effect to make sure graph.eliminate_dead_code doesn't remove the calls by accident. Test Plan: Add a new test pytest test/export/test_torchbind.py -k test_export_inplace_custom_op Differential Revision: [D59498728](https://our.internmc.facebook.com/intern/diff/D59498728) Pull Request resolved: pytorch#129680 Approved by: https://github.com/angelayi
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 (oldest at bottom):
Before the PR, custom ops that don't return outputs will get eliminated after calling
.module()because the effect_token that keeps the operator alive is removed in remove_effect_token pass. The reason why we want to remove_effect_token is because we don't want the token to be part of input. However, this causes DCE calls in remove_effect_token itself and the dce calls in unlift to remove the custom op in the graph causing an error in the exported graph.This PR calls has_side_effect in with_effect to make sure graph.eliminate_dead_code doesn't remove the calls by accident.
Test Plan:
Add a new test pytest test/export/test_torchbind.py -k test_export_inplace_custom_op
Differential Revision: D59498728