Skip to content

Adding support for inlining if branches#10084

Closed
eellison wants to merge 4 commits intopytorch:masterfrom
eellison:constant_if_support
Closed

Adding support for inlining if branches#10084
eellison wants to merge 4 commits intopytorch:masterfrom
eellison:constant_if_support

Conversation

@eellison
Copy link
Contributor

@eellison eellison commented Jul 31, 2018

Inlining if branches which have constant inputs. If an if node gets inlined, the set of mutated variables returned by its ancestors may have changed. In the following example the block should
return a mutated set of (a) and not (a, b).

if cond:
  if True:
	 a = a - 1
    else:
	b = b - 1

To calculate this we recursively update mutate variables in if branches from the leaf nodes up.

@eellison
Copy link
Contributor Author

eellison commented Jul 31, 2018

@pytorchbot retest this please

1 similar comment
@eellison
Copy link
Contributor Author

eellison commented Aug 1, 2018

@pytorchbot retest this please

@eellison
Copy link
Contributor Author

eellison commented Aug 2, 2018

@pytorchbot retest this please

1 similar comment
@eellison
Copy link
Contributor Author

eellison commented Aug 2, 2018

@pytorchbot retest this please

@zdevito zdevito added the oncall: jit Add this issue/PR to JIT oncall triage queue label Aug 3, 2018
@eellison
Copy link
Contributor Author

eellison commented Aug 3, 2018

@pytorchbot retest this please

zdevito
zdevito reviewed Aug 3, 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.

I have some questions about how the recomputeMutatedVariables works inline. I think it can be made significantly simpler.

torch/csrc/jit/passes/constant_propagation.cpp Outdated
};

for (Node *orig : body->nodes()) {
Node *clone = graph->insertNode(graph->createClone(orig, get_value));

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/passes/constant_propagation.cpp Outdated
return maybe_value && *maybe_value;
}

void lowerIf(Node *n) {

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/passes/constant_propagation.cpp Outdated
}

//returns true if the mutated variables are changed
bool recomputeMutatedVariables(Node *n) {

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/passes/constant_propagation.cpp Outdated
auto initial_outputs = true_block->outputs().size();
for (size_t i = 0; i < true_block->outputs().size();) {
//neither block mutates output i
if (!mutated_variables.count(true_block->outputs()[i]) &&

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/passes/constant_propagation.cpp Outdated
if (constant_inputs) {
lowerIf(n);
} else if (changed) {
changed = recomputeMutatedVariables(n);

This comment was marked as off-topic.

Sign in to view

@eellison eellison force-pushed the constant_if_support branch from ae421d1 to ac365bb Compare August 6, 2018 16:52
@eellison eellison force-pushed the constant_if_support branch from ac365bb to 2d854fb Compare August 6, 2018 18:52
@eellison
Copy link
Contributor Author

eellison commented Aug 6, 2018

@pytorchbot retest this please

4 similar comments
@eellison
Copy link
Contributor Author

eellison commented Aug 6, 2018

@pytorchbot retest this please

@eellison
Copy link
Contributor Author

eellison commented Aug 7, 2018

@pytorchbot retest this please

@eellison
Copy link
Contributor Author

eellison commented Aug 7, 2018

@pytorchbot retest this please

@eellison
Copy link
Contributor Author

eellison commented Aug 7, 2018

@pytorchbot retest this please

@eellison eellison force-pushed the constant_if_support branch from 2d854fb to 55bf9a4 Compare August 7, 2018 21:21
@eellison
Copy link
Contributor Author

eellison commented Aug 7, 2018

@pytorchbot retest this please

1 similar comment
@eellison
Copy link
Contributor Author

eellison commented Aug 8, 2018

@pytorchbot retest this please

@yf225
Copy link
Contributor

yf225 commented Aug 14, 2018

@eellison @zdevito What should be the next step for this patch?

zdevito
zdevito approved these changes Aug 15, 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 now. One minor thing about cleaning up prim::Loop loop-carried dependencies, but the patch is still correct without it and can be merged.

torch/csrc/jit/passes/constant_propagation.cpp
if (constant_inputs) {
inlineIf(n);
} else {
removeExtraNodeOutputs(n);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

facebook-github-bot
facebook-github-bot reviewed Aug 15, 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.

apaszke
apaszke approved these changes Aug 15, 2018
View reviewed changes
torch/csrc/jit/passes/constant_propagation.cpp Outdated
WithInsertPoint guard(n);
for (size_t i = 0; i < outputs.size(); ++i) {
auto new_output = graph->insertConstant(outputs[i]);
auto new_output = insertConstant(*graph, outputs[i]);

This comment was marked as off-topic.

Sign in to view

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

bool isTrueConstant(Value *val) {
at::optional<bool> maybe_value = constant_as<bool>(val);
return maybe_value && *maybe_value;

This comment was marked as off-topic.

Sign in to view

facebook-github-bot
facebook-github-bot reviewed Aug 16, 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 Aug 16, 2018

@pytorchbot retest this please

@eellison
Copy link
Contributor Author

eellison commented Aug 16, 2018

@pytorchbot retest this please

1 similar comment
@eellison
Copy link
Contributor Author

eellison commented Aug 16, 2018

@pytorchbot retest this please

facebook-github-bot
facebook-github-bot reviewed Aug 17, 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

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

@zdevito zdevito zdevito approved these changes

@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 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

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