[NNC] Remove VarBinding and go back to Let stmts#42634
[NNC] Remove VarBinding and go back to Let stmts#42634nickgg wants to merge 1 commit intopytorch:masterfrom
Conversation
ZolotukhinM
left a comment
There was a problem hiding this comment.
Looks good to me! Might be worth checking if others are ok with that change too though.
There was a problem hiding this comment.
Should we var_by_scope_.erase(scope_) as well?
There was a problem hiding this comment.
Probably, for correctness. Doesn't really matter since we will never recur back here in the same invocation, but why not.
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 8f10233 (more details on the Dr. CI page):
ci.pytorch.org: 1 failedThis 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 8 times. |
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: Awhile back when commonizing the Let and LetStmt nodes, I ended up removing both and adding a separate VarBinding section the Block. At the time I couldn't find a counter example, but I found it today: Local Vars and Allocations dependencies may go in either direction and so we need to support interleaving of those statements. So, I've removed all the VarBinding logic and reimplemented Let statements. ZolotukhinM I think you get to say "I told you so". No new tests, existing tests should cover this. Pull Request resolved: pytorch#42634 Reviewed By: mruberry Differential Revision: D22969771 Pulled By: nickgg fbshipit-source-id: a46c5193357902d0f59bf30ab103fe123b1503f1
Awhile back when commonizing the Let and LetStmt nodes, I ended up removing both and adding a separate VarBinding section the Block. At the time I couldn't find a counter example, but I found it today: Local Vars and Allocations dependencies may go in either direction and so we need to support interleaving of those statements.
So, I've removed all the VarBinding logic and reimplemented Let statements. @ZolotukhinM I think you get to say "I told you so". No new tests, existing tests should cover this.