[jit] Fix alias analysis for fork/wait #16671
Conversation
facebook-github-bot
added
the
oncall: jit
| // | ||
| // In reality, any reads *inside* the live interval are undefined behavior, | ||
| // since the writes may or may not have been executed yet. But we'll let | ||
| // users do that and shoot themselves in the foot for now. |
There was a problem hiding this comment.
Do we want to print a warning in this situation?
There was a problem hiding this comment.
I'm reluctant to incorporate alias analysis results in user-visible error reporting. We can't expect users to know about alias info/the IR/how we've mutated the graph up till now, so an error msg would be super confusing and probably not actionable.
| } | ||
|
|
||
| void AliasDb::analyzeFork(Node* node) { | ||
| // We treat forks as if their subgraph ran immediately. Since the alias |
There was a problem hiding this comment.
Maybe rephrase ? We're treating forks as potentially running any time being fork & wait
| const auto subgraphWrites = getWrites(subgraph->block()); | ||
| for (const auto& use : fut->uses()) { | ||
| const auto wait = use.user; | ||
| AT_ASSERT(wait->outputs().size() == subgraph->outputs().size()); |
There was a problem hiding this comment.
Will this fail is it's used as an output in an if node ?
There was a problem hiding this comment.
Hmm…good point. I guess it's non-trivial to find all the matching "waits".
suo
force-pushed
the
suo/fix-fork
branch
2 times, most recently
from
4efdc6d to
ed64062
Compare
There was a problem hiding this comment.
@suo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@suo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
suo
force-pushed
the
suo/fix-fork
branch
2 times, most recently
from
847268c to
847bcdb
Compare
There was a problem hiding this comment.
@suo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| return shouldAnnotate(v->type()); | ||
| } | ||
|
|
||
| std::unordered_set<Node*> getMatchingWaits(Value* fut) { |
There was a problem hiding this comment.
haven't taken full look at new changes but It doesn't look like this is being called except recursively
There was a problem hiding this comment.
blah, this was left in, will remove!
There was a problem hiding this comment.
@suo has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@suo is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
This reverts commit 43f4c86.
(review top commit only).
As expected, fork/wait introduces some corner cases into the alias analysis. The comments inline should describe the changes.