Skip to content

Pool constants during script compilation.#10231

Closed
resistor wants to merge 1 commit intopytorch:masterfrom
resistor:constant-pooling
Closed

Pool constants during script compilation.#10231
resistor wants to merge 1 commit intopytorch:masterfrom
resistor:constant-pooling

Conversation

@resistor
Copy link
Contributor

@resistor resistor commented Aug 4, 2018

This places all constants in the entry block of the graph, and de-duplicates them.

@resistor
Copy link
Contributor Author

resistor commented Aug 4, 2018

@zdevito

This comment was marked as off-topic.

This comment was marked as off-topic.

@zou3519 zou3519 added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 14, 2018
@resistor resistor force-pushed the constant-pooling branch 2 times, most recently from 10e5de4 to fbcba68 Compare August 14, 2018 20:05
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 is good, but it has submodule checkins for NNPACK, cpuinfo, and ONNX that need to be reverted.

@resistor
Copy link
Contributor Author

@zdevito Fixed

@resistor resistor force-pushed the constant-pooling branch 3 times, most recently from 9b9366e to a1289f8 Compare August 30, 2018 18:43
Copy link
Contributor

@apaszke apaszke left a comment

Choose a reason for hiding this comment

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

Can't we implement this as a pass? There are many ways in which we introduce the constants into the graphs (both tracing and most of our rewrites do so!), and it would be good to fix those as well.

@resistor
Copy link
Contributor Author

CSE already does the easy cases, but handling it generally requires full data flow analysis and code motion. It’s much easier and more efficient to avoid generating redundant comments in the first place.

@resistor
Copy link
Contributor Author

FWIW, this is exactly why constants are not instructions in LLVM IR.

This places all constants in the entry block of the graph, and de-duplicates them.
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.

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

PenghuiCheng pushed a commit to PenghuiCheng/pytorch that referenced this pull request Sep 11, 2018
Summary:
This places all constants in the entry block of the graph, and de-duplicates them.
Pull Request resolved: pytorch#10231

Differential Revision: D9601501

Pulled By: resistor

fbshipit-source-id: daa10ed8c99e9894830d6f3e5d65c8d3ab5ea899
@eellison eellison mentioned this pull request Oct 1, 2018
facebook-github-bot pushed a commit that referenced this pull request Oct 8, 2018
Summary:
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.
Pull Request resolved: #12222

Reviewed By: driazati

Differential Revision: D10201616

Pulled By: eellison

fbshipit-source-id: bc9c5be26868c8b5414257a0d4462de025aeb9bd
@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

Labels

oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants