[JIT] fix common_expression_hoisting #74794
Conversation
aliasdb check was incorrect, caused issues in #74056 This checks whether it is safe to move before the node at the beginning of the if branch instead of after the node at the beginning of the if branch [ghstack-poisoned]
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 184f81d (more details on the Dr. CI page): 💚 💚 Looks good so far! There are no failures yet. 💚 💚 This comment was automatically generated by Dr. CI (expand for details).Please report bugs/suggestions to the (internal) Dr. CI Users group. |
facebook-github-bot
added
the
oncall: jit
aliasdb check was incorrect, caused issues in #74056 This checks whether it is safe to move before the node at the beginning of the if branch instead of after the node at the beginning of the if branch [ghstack-poisoned]
aliasdb check was incorrect, caused issues in #74056 This checks whether it is safe to move before the node at the beginning of the if branch instead of after the node at the beginning of the if branch [ghstack-poisoned]
|
@davidberard98 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
davidberard98
requested review from
Gamrix and
eellison
and removed request for
Gamrix and
eellison
|
Turns out previous fix is insufficient, here's a new repro that still fails import torch
from typing import Dict
def fn(x, y, cond: bool, d: Dict[str, torch.Tensor]):
if cond:
m = x.relu()
f1 = torch.rand((2, 2))
d["test"] = f1
z = d["test"]
else:
m = y.gelu()
f2 = torch.rand((3, 2))
d["test"] = f2
z = d["test"]
return m, z
fn_s = torch.jit.script(fn)
x = torch.rand((2, 2))
y = torch.rand((2, 2))
d = {"x": x}
fn_s(x, y, 1, d) |
|
The fix looks good, though yes, the new repro is still failing. |
|
From your repo, |
aliasdb check was incorrect, caused issues in #74056 This checks whether it is safe to move before the node at the beginning of the if branch instead of after the node at the beginning of the if branch Differential Revision: [D35188927](https://our.internmc.facebook.com/intern/diff/D35188927) [ghstack-poisoned]
aliasdb check was incorrect, caused issues in #74056 This checks whether it is safe to move before the node at the beginning of the if branch instead of after the node at the beginning of the if branch Differential Revision: [D35188927](https://our.internmc.facebook.com/intern/diff/D35188927) [ghstack-poisoned]
|
@Gamrix In the case of the 2nd repro, we try to move the So then we were moving |
aliasdb check was incorrect, caused issues in #74056 This checks whether it is safe to move before the node at the beginning of the if branch instead of after the node at the beginning of the if branch Differential Revision: [D35188927](https://our.internmc.facebook.com/intern/diff/D35188927) [ghstack-poisoned]
| // node. If it ended up at the very beginning of the if block, then it's | ||
| // safe to move it outside. | ||
| if (true_b_node != true_block->nodes().front() || | ||
| false_b_node != false_block->nodes().front()) { |
There was a problem hiding this comment.
This is not safe, as you can have two separate lines of code that both can be moved forward, but not be at the front. This in turn can cause the program to loop infinitely due to the iteration order of the for loop.
There was a problem hiding this comment.
How would the loop happen?
If we have nodes n_1, n_2, ..., n_i, n_{i+1}, ..., n_m and start the loop at n_i:
- store n_i as
false_b_nodeincrement iterator to n_{i+1} - possibly scramble n_1, n_2, ... n_i (working set can only contain nodes in between toMove node and target node)
- repeat
There was a problem hiding this comment.
I think it's guaranteed that n_{i+1}, ... , n_m are unaffected by this comment
pytorch/torch/csrc/jit/ir/alias_analysis.h
Lines 115 to 123 in 9872a06
There was a problem hiding this comment.
You're right, I forgot that we already incremented the iterator before we did all the moves. You'd think I would know my own code :/ .
|
@davidberard98 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@davidberard98 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: Pull Request resolved: #74794 aliasdb check was incorrect, caused issues in #74056 This checks whether it is safe to move before the node at the beginning of the if branch instead of after the node at the beginning of the if branch Test Plan: Imported from OSS Reviewed By: Gamrix Differential Revision: D35188927 Pulled By: davidberard98 fbshipit-source-id: 722ecf67840a066dbee55b4033c35122a20f368e
Stack from ghstack:
aliasdb check was incorrect, caused issues in #74056
This checks whether it is safe to move before the node at the beginning
of the if branch instead of after the node at the beginning of the if
branch
Differential Revision: D35188927