graph fuser inserts explicit expands where necessary#10325
graph fuser inserts explicit expands where necessary#10325zou3519 wants to merge 3 commits intopytorch:masterfrom
Conversation
|
need rebase. mac seems broken as well |
zdevito
left a comment
There was a problem hiding this comment.
In general, the approach makes sense but it needs some work to make the expand-generation logic less intrusive in the fusion pass. Right now it makes it increasingly difficult to make further edits to the pass. It might make sense to just establish when FusionGroups should be made and then do a followup pass to issue expands that will occur before the FusionGroup.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
3453b27 to
943fcf2
Compare
|
I've made the following changes based on @zdevito's comments:
This should be more robust than the previous approach, but I'm not sure if the expand-generation logic is still too intrusive or not. In addition, @apaszke's upcoming refactor of the graph fuser probably will fix this exact problem, so I am also fine with punting on this PR until the refactor and seeing how the state of things are then. |
apaszke
left a comment
There was a problem hiding this comment.
Minor errors that should be fixed before this lands, but generally LGTM.
torch/csrc/jit/graph_executor.cpp
Outdated
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
facebook-github-bot
left a comment
There was a problem hiding this comment.
zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Fixes pytorch#10096 If the only thing preventing a simple mappable operator from being fused into a fusion group is that its Tensor inputs are not of the same shape as the output, then the graph fuser inserts explicit expand nodes for those inputs. This helps the graph fuser not miss out on any fusion opportunities involving simple mappable operations that have Tensor inputs. The main changes are: - isFusable(Node) doesn't check input sizes anymore. A simple mappable operation with correct input/output types is fusible. - shouldFuse(consumer, producer) checks consumer and producer for matching map_size. - fuse(consumer, producer) emits expand nodes as necessary - tryMoveChunk(consumer, chunk) also emits expand nodes as necessary because isFusable(Node) doesn't check input sizes anymore. A chunk should only be moved if the move results in a node that can be fused with consumer.
64148e8 to
61cc7d6
Compare
facebook-github-bot
left a comment
There was a problem hiding this comment.
zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@pytorchbot retest this please |
Fixes #10096
If the only thing preventing a simple mappable operator from being fused
into a fusion group is that its Tensor inputs are not of the same shape as the
output, then the graph fuser inserts explicit expand nodes for those
inputs.
This helps the graph fuser not miss out on any fusion opportunities
involving simple mappable operations that have Tensor inputs. This PR
doesn't do anything for the scalar case; that can be addressed later.
Test Plan
forwards pass by allowing more operations to be fused into the LSTMCell's single
FusionGroup.
cc @apaszke @zdevito