Skip to content

exclude mutated buffer #2876

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

Closed
wants to merge 1 commit into from
Closed

Conversation

cccclai
Copy link
Contributor

@cccclai cccclai commented Apr 5, 2024

Differential Revision: D55812844

Copy link

pytorch-bot bot commented Apr 5, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit 6e049ff with merge base 86b326a (image):
💚 Looks good so far! There are no failures yet. 💚

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

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 5, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D55812844

@@ -515,6 +515,12 @@ def tag_constant_data(edge_program: ExportedProgram) -> None:
or is_buffer(edge_program, node)
or is_lifted_tensor_constant(edge_program, node)
):
for node_user in node.users:
Copy link
Collaborator

@YifanShenSZ YifanShenSZ Apr 5, 2024

Choose a reason for hiding this comment

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

Shall we make this an option, rather than hard coded?

Concretely, shall we revise line 503 to

def tag_constant_data(edge_program: ExportedProgram, exclude_mutable_buffer: bool = False) -> None:

so it is still backward compatible, while giving us the flexibility to choose including / excluding mutable buffer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This util function is to tag constant data, while mutable buffer isn't really considered as a constant. I think the backend will know more precisely what kind of mutate operator they can support, and the custom logic can live inside the partitioner.

@cccclai cccclai force-pushed the export-D55812844 branch from 8fd03e9 to d3adb47 Compare April 7, 2024 21:10
cccclai added a commit to cccclai/executorch-1 that referenced this pull request Apr 7, 2024
Summary: Pull Request resolved: pytorch#2876

Differential Revision: D55812844
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D55812844

cccclai added a commit to cccclai/executorch-1 that referenced this pull request Apr 7, 2024
Summary: Pull Request resolved: pytorch#2876

Differential Revision: D55812844
@cccclai cccclai force-pushed the export-D55812844 branch from d3adb47 to cd2af09 Compare April 7, 2024 21:15
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D55812844

Summary: Pull Request resolved: pytorch#2876

Differential Revision: D55812844
@cccclai cccclai force-pushed the export-D55812844 branch from cd2af09 to 6e049ff Compare April 7, 2024 21:22
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D55812844

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 599cfde.

@cccclai
Copy link
Contributor Author

cccclai commented Apr 9, 2024

@pytorchbot cherry-pick --onto release/0.2 -c critical

pytorchbot pushed a commit that referenced this pull request Apr 9, 2024
Summary:
Pull Request resolved: #2876

Fixing the tag constant for mutable buffer. The buffer shouldn't be tagged if it's going to be mutated by the delegated. It's more common in hardware backends

Will follow up and test having delegate consume mutation

Reviewed By: mcr229, angelayi

Differential Revision: D55812844

fbshipit-source-id: e0be4c2dc295141d673cccb1aeecee45894b1e70
(cherry picked from commit 599cfde)
@pytorchbot
Copy link
Collaborator

Cherry picking #2876

The cherry pick PR is at #2946 and it is recommended to link a critical cherry pick PR with an issue

Details for Dev Infra team Raised by workflow job

@cccclai cccclai mentioned this pull request Apr 9, 2024
mergennachin pushed a commit that referenced this pull request Apr 9, 2024
Summary:
Pull Request resolved: #2876

Fixing the tag constant for mutable buffer. The buffer shouldn't be tagged if it's going to be mutated by the delegated. It's more common in hardware backends

Will follow up and test having delegate consume mutation

Reviewed By: mcr229, angelayi

Differential Revision: D55812844

fbshipit-source-id: e0be4c2dc295141d673cccb1aeecee45894b1e70
(cherry picked from commit 599cfde)
This was referenced Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants