[PT2][memory] add missing dependencies due to mutations #153569
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/153569
Note: Links to docs will display an error until the docs builds have been completed. ❗ 1 Active SEVsThere are 1 currently active SEVs. If your PR is affected, please view them below: ✅ No FailuresAs of commit 65d4a4e with merge base e802b29 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@pytorchbot label "topic: not user facing" |
torch/_inductor/memory.py
Outdated
| if dep.name in name_to_buf: | ||
| buf = name_to_buf[dep.name] | ||
| if buf.get_mutations(): | ||
| mutated_buf_name = buf.get_mutations()[0] | ||
| dep_name_to_succ_nodes[mutated_buf_name].add(node) |
There was a problem hiding this comment.
These should already be modeled via weak deps. can we use those instead ?
There was a problem hiding this comment.
Do you have a code pointer for get to a node's weak dependency? Seems that by the time of the memory reorder pass, weak dependencies are pruned?
There was a problem hiding this comment.
Could we re-add them generically so that they work for all of the reordering passes, not just memory ? cc @BoyuanFeng who ran into this recently as well.
There was a problem hiding this comment.
Could you add in compute_dependencies()?
Also, what is the unit test to repro the issue?
There was a problem hiding this comment.
Hmmm, I was not aware of this function, thanks for the link and let me check on this.
For repro and unit test, I don't have one yet. I will write one that is related to mutations.
This PR adds the missing data dependency due to mutations (illustrated in the figure below).  See [this doc](https://docs.google.com/document/d/1lkKulwIb-fYL_p8jn1SD6Lh1PoAKBgpBsU5sAH80leI/edit?tab=t.0#bookmark=id.w3n4k1y4rdz8) with testing details [internal only] cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov [ghstack-poisoned]
This PR adds the missing data dependency due to mutations (illustrated in the figure below).  See [this doc](https://docs.google.com/document/d/1lkKulwIb-fYL_p8jn1SD6Lh1PoAKBgpBsU5sAH80leI/edit?tab=t.0#bookmark=id.w3n4k1y4rdz8) with testing details [internal only] cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov [ghstack-poisoned]
This PR adds the missing data dependency due to mutations (illustrated in the figure below).  See [this doc](https://docs.google.com/document/d/1lkKulwIb-fYL_p8jn1SD6Lh1PoAKBgpBsU5sAH80leI/edit?tab=t.0#bookmark=id.w3n4k1y4rdz8) with testing details [internal only] cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov [ghstack-poisoned]
Stack from ghstack (oldest at bottom):
This PR adds the missing data dependency due to mutations (illustrated in the figure below).
See this doc with testing details [internal only]
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @chenyang78 @kadeng @muchulee8 @amjames @chauhang @aakhundov