Simplify expr before getting implications in _maybe_evaluate_static #135499
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/135499
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit 65b574e with merge base 356f14e ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
pytorch-bot
bot
added
ciflow/inductor
module: dynamo
release notes: fx
Fixes #134268 Previously we weren't simplifying these expressions before turning them into runtime asserts, resulting in inconsistent application of FloorDiv/CleanDiv. See #134268 for more details. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames rec [ghstack-poisoned]
bobrenjc93
changed the title
…te_static" Fixes #134268 Previously we weren't simplifying these expressions before calling get_implications, resulting in inconsistent application of FloorDiv/CleanDiv. See #134268 for more details. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames rec [ghstack-poisoned]
…te_static" Fixes #134268 Previously we weren't simplifying these expressions before calling get_implications, resulting in inconsistent application of FloorDiv/CleanDiv. See #134268 for more details. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames rec [ghstack-poisoned]
|
One downside to this approach is that I have a performance optimization pass that avoids having to continuously rebuild implications by maintaining a dict of axioms that is added to as we discover new guards. Here, I can't conveniently simplify the expr before getting implications, because I'm no longer continuously rebuilding the dict. |
|
@ezyang Do you have a link to that PR? The trickiness stems from the dependence on self.divisible within simplify. My intuition tells me the optimal answer may involve a dependency graph. Naively perhaps every time we update self.divisible, we would update the dict of axioms? Or perhaps there is some smarter finer grain fanout calculation that can be done. I'm open to a trying different approach if you have one in mind. I'll defer to you w.r.t. the importance of correctness vs performance, but intuitively it seems we should ensure the former over the latter. Or in other words it might make sense to land this as is and in the optimization diff we can rework the implementation and keep the test? Let me know how you'd like to proceed! |
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 1 jobs have failed, first few of them are: trunk / macos-py3-arm64 / build Details for Dev Infra teamRaised by workflow job |
…te_static" Fixes #134268 Previously we weren't simplifying these expressions before calling get_implications, resulting in inconsistent application of FloorDiv/CleanDiv. See #134268 for more details. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames rec [ghstack-poisoned]
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
This durably regresses instruction count on update_hint_regression by 2.2% (https://fburl.com/unidash/v36uhdz4), which is not surprising as we're doing more simplification than we did before. Since this is a correctness fix I think we'll eat it but it's something to keep an eye on. cc @isuruf |
|
#135863 might help |
…ytorch#135499) Fixes pytorch#134268 Previously we weren't simplifying these expressions before calling get_implications, resulting in inconsistent application of FloorDiv/CleanDiv. See pytorch#134268 for more details. Pull Request resolved: pytorch#135499 Approved by: https://github.com/ezyang
4 participants
Stack from ghstack (oldest at bottom):
Fixes #134268
Previously we weren't simplifying these expressions before calling get_implications, resulting in inconsistent application of FloorDiv/CleanDiv. See #134268 for more details.
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames @rec