[NNC] Add simplification of Loop + Condition patterns. #44764
Conversation
facebook-github-bot
added
the
oncall: jit
Codecov Report
@@ Coverage Diff @@
## master #44764 +/- ##
==========================================
- Coverage 68.08% 68.08% -0.01%
==========================================
Files 384 384
Lines 49774 49776 +2
==========================================
+ Hits 33890 33891 +1
- Misses 15884 15885 +1
Continue to review full report at Codecov.
|
navahgar
left a comment
navahgar
left a comment
There was a problem hiding this comment.
Mostly looks good to me. Just one test case that needs a fix.
There was a problem hiding this comment.
Why isn't reordering better here? Bcos we need to have two copies of the for loop, which increases compile time?
There was a problem hiding this comment.
Yeah pretty much increases to code size. It's probably better in most cases but I'm not 100% confident it's better or neutral in all cases.
What do you think?
There was a problem hiding this comment.
I can imagine a worst-case scenario where the code size could become a big issue. And since simplify silently does these changes, I agree with your choice to not support it, until it becomes a necessity.
torch/csrc/jit/tensorexpr/stmt.h
Outdated
There was a problem hiding this comment.
nit: why not have the same name as the function below, cloneWithNewBody?
There was a problem hiding this comment.
Because it has two bodies! (true_stmt and false_stmt) whereas the one below only has a single body.
There was a problem hiding this comment.
I was thinking about having the same name and have the two body version as an overload. Never mind.
nickgg
force-pushed
the
condReordering2
branch
from
6c6bc2a to
97e7c76
Compare
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@nickgg has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
💊 CI failures summary and remediationsAs of commit 7d5f3ec (more details on the Dr. CI page): ✅ None of the CI failures appear to be your fault 💚
🚧 1 ongoing upstream failure:These were probably caused by upstream breakages that are not fixed yet: This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 13 times. |
nickgg
force-pushed
the
condReordering2
branch
from
97e7c76 to
2c52f7c
Compare
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@nickgg has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
nickgg
force-pushed
the
condReordering2
branch
from
2c52f7c to
7d5f3ec
Compare
facebook-github-bot
left a comment
facebook-github-bot
left a comment
There was a problem hiding this comment.
@nickgg has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary:
Adds a new optimization to the IRSimplifier which changes this pattern:
```
for ...
if ...
do thing;
```
into:
```
if ...
for ...
do thing;
```
Which should be almost strictly better.
There are many cases where this isn't safe to do, hence tests. Most obviously when the condition depends on something modified within the loop.
Pull Request resolved: #44764
Reviewed By: mruberry
Differential Revision: D23734463
Pulled By: nickgg
fbshipit-source-id: 51617e837de96b354fb702d0090ac65ddc523d36
4 participants
Adds a new optimization to the IRSimplifier which changes this pattern:
into:
Which should be almost strictly better.
There are many cases where this isn't safe to do, hence tests. Most obviously when the condition depends on something modified within the loop.