Skip to content

constant pooling pass#12222

Closed
eellison wants to merge 5 commits intopytorch:masterfrom
eellison:constant_pooling_pass
Closed

constant pooling pass#12222
eellison wants to merge 5 commits intopytorch:masterfrom
eellison:constant_pooling_pass

Conversation

@eellison
Copy link
Contributor

@eellison eellison commented Oct 1, 2018

Add a pass to move all constants to the beginning of the graph, and deduplicate.

This extends #10231 to also handle constants introduced in inlining, constant propagation, etc.

zdevito
zdevito approved these changes Oct 1, 2018
View reviewed changes
Copy link
Contributor

@zdevito zdevito left a comment

Choose a reason for hiding this comment

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

This looks good I just have minor nits. I think we should run this in compiler.cpp so that it cleans up graphs before we ever have to debug them with duplicated constants.

torch/csrc/jit/node_hashing.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/node_hashing.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/node_hashing.h Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/passes/constant_pooling.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

test/test_jit.py Outdated

This comment was marked as off-topic.

Sign in to view

@eellison eellison force-pushed the constant_pooling_pass branch 3 times, most recently from d6737b8 to a2644a5 Compare October 3, 2018 17:38
apaszke
apaszke reviewed Oct 3, 2018
View reviewed changes
torch/csrc/jit/graph_executor.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/passes/node_hashing.h Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/script/compiler.cpp Outdated

This comment was marked as off-topic.

Sign in to view

@eellison eellison force-pushed the constant_pooling_pass branch from 529a186 to 8f3d1a1 Compare October 4, 2018 17:10
facebook-github-bot
facebook-github-bot reviewed Oct 4, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@eellison
Copy link
Contributor Author

eellison commented Oct 4, 2018

@pytorchbot retest this please

1 similar comment
@eellison
Copy link
Contributor Author

eellison commented Oct 4, 2018

@pytorchbot retest this please

apaszke
apaszke reviewed Oct 5, 2018
View reviewed changes
torch/csrc/jit/graph_executor.cpp
void runOptimization(std::shared_ptr<Graph>& graph, const ArgumentSpec& spec) {
EliminateDeadCode(graph);
EliminateCommonSubexpression(graph);
ConstantPooling(graph);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@eellison eellison force-pushed the constant_pooling_pass branch from 8f3d1a1 to 3ae8454 Compare October 5, 2018 22:48
facebook-github-bot
facebook-github-bot reviewed Oct 5, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@eellison eellison force-pushed the constant_pooling_pass branch from 3ae8454 to ecfc98c Compare October 5, 2018 23:57
facebook-github-bot
facebook-github-bot reviewed Oct 5, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

eellison has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zdevito zdevito zdevito approved these changes

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@colesbury colesbury Awaiting requested review from colesbury

@ezyang ezyang Awaiting requested review from ezyang

@gchanan gchanan Awaiting requested review from gchanan

@soumith soumith Awaiting requested review from soumith

+1 more reviewer

@apaszke apaszke apaszke left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@eellison @zdevito @apaszke @facebook-github-bot @ezyang