Skip to content

[NNC] Fuse identical conditions in simplifier #44886

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.

Sign up for GitHub

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

Jump to bottom
Closed
nickgg wants to merge 1 commit into from
Closed

[NNC] Fuse identical conditions in simplifier #44886

nickgg wants to merge 1 commit into from

Conversation

@nickgg
Copy link
Contributor

@nickgg nickgg commented Sep 17, 2020

Adds a pass to the IR Simplifier which fuses together the bodies of Cond statements which have identical conditions. e.g.

if (i < 10) {
  do_thing_1;
} else {
  do_thing_2;
}
if (i < 10) {
  do_thing_3;
}

is transformed into:

if (i < 10) {
  do_thing_1;
  do_thing_3;
} else {
  do_thing_2;
}

@nickgg nickgg requested a review from apaszke as a code owner September 17, 2020 16:32
navahgar
navahgar approved these changes Sep 17, 2020
View reviewed changes
Copy link
Contributor

@navahgar navahgar left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me. Just a couple minor corrections in tests.

test/cpp/tensorexpr/test_simplify.cpp Outdated
Copy link
Contributor

@navahgar navahgar Sep 17, 2020

Choose a reason for hiding this comment

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

s/j/i to make sure that simplification does not happen because of 10 != 11.

test/cpp/tensorexpr/test_simplify.cpp Outdated
Copy link
Contributor

@navahgar navahgar Sep 17, 2020

Choose a reason for hiding this comment

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

s/j/i - same as above to make sure that LT v GT is the problem here.

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 17, 2020
@codecov
Copy link

codecov bot commented Sep 17, 2020 •
edited
Loading

Codecov Report

Merging #44886 into master will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #44886      +/-   ##
==========================================
- Coverage   67.90%   67.89%   -0.01%     
==========================================
  Files         384      384              
  Lines       49890    49890              
==========================================
- Hits        33876    33873       -3     
- Misses      16014    16017       +3
Impacted Files Coverage Δ
torch/utils/_benchmark/utils/common.py 77.68% <0.00%> (-1.66%) ⬇️
torch/testing/_internal/expecttest.py 77.55% <0.00%> (-1.03%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d57025...00e48b3. Read the comment docs.

@nickgg
Copy link
Contributor Author

nickgg commented Sep 17, 2020

Good catches, thanks.

facebook-github-bot
facebook-github-bot reviewed Sep 17, 2020
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.

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

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 18, 2020

@nickgg merged this pull request in f175830.

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

@bertmaher bertmaher Awaiting requested review from bertmaher

@ZolotukhinM ZolotukhinM Awaiting requested review from ZolotukhinM

@apaszke apaszke Awaiting requested review from apaszke

+1 more reviewer

@navahgar navahgar navahgar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Merged 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.

4 participants

@nickgg @facebook-github-bot @navahgar @mruberry