Rewrite maybe_reduce more carefully for unbacked SymInt #119562
Conversation
TODO: Restore the NT logic Signed-off-by: Edward Z. Yang <[email protected]> [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/119562
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit efcdc78 with merge base 24bdd03 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
github-actions
bot
requested review from
SherlockNoMad,
antoniojkim,
bdhirsh and
miladm
Fixes #119476 TODO: Restore the NT logic Signed-off-by: Edward Z. Yang <ezyangmeta.com> [ghstack-poisoned]
ezyang
changed the title
Fixes #119476 Signed-off-by: Edward Z. Yang <ezyangmeta.com> [ghstack-poisoned]
Fixes #119476 Signed-off-by: Edward Z. Yang <ezyangmeta.com> [ghstack-poisoned]
|
This is ready |
ezyang
added
ciflow/trunk
| return at::zeros_symint(shape_as_dim_vector(), options_); | ||
| } | ||
|
|
||
| static bool definitely_true(const c10::SymBool& b) { |
There was a problem hiding this comment.
Should this be defined in the SymInt header itself? (maybe as a macro to be able to preserve file/line)
| fail(); | ||
| } | ||
| bool needs_reduce = false; | ||
| for (const auto i : c10::irange(ndim)) { |
There was a problem hiding this comment.
I guess we can't make at::is_expandable_to work with unbacked SymInt?
There was a problem hiding this comment.
The problem is that is_expandable_to is returning a true/false quantity, which may or may not cause an error condition. However, I can only produce a deferred runtime assert if, at the time I try to guard on a boolean expression, I know that if this expression evaluates to false it will be an error condition. Which is_expandable_to doesn't! This is why everything needed to be inlined here.
There was a problem hiding this comment.
Is that what SymBool can solve?
But I guess I would need to understand the unbaked symint strategy to see what is feasible here.
There was a problem hiding this comment.
Returning a SymBool could help. However, now you have to rewrite is_expandable_to to be data oblivious: it's not allowed to do an early return when it notices that something is not expandable to.
Fixes #119476 Signed-off-by: Edward Z. Yang <ezyangmeta.com> cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 aakhundov kadeng [ghstack-poisoned]
|
updated |
albanD
left a comment
albanD
left a comment
There was a problem hiding this comment.
Sounds good to merge. My question is mostly because I'm not clear on what is feasible.
|
@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 |
…ce (#154172) This was added in #119562 the idea in this loop seems to be the following. ``` if (TORCH_GUARD_SIZE_OBLIVIOUS(size.sym_eq(1))) { // NB: we could short circuit this once needs_reduce is true but there's // no point since the reduction function will guard on this anyway if (!c10::guard_or_false(size.sym_eq(target), __FILE__, __LINE__)) { needs_reduce = true; } } else { if (!size.sym_eq(target).expect_true(__FILE__, __LINE__)) { fail(); } } ``` 1. if we know size ==1 1.1 : if we know for sure size == target --> no reduce needed. 1.2 : we know for sure that size != target --> we do reduction. 1.3: we could not tell if size == target or not --> we do reduction. 2. if we do now know if size ==1 or not we add a runtime assertions that size ==target and we fail at runtime if size is not equal to target. We could have simplified 1.1 and always do reduction under 1.1, since doing 1.3 without runtime checks implies that it is safe, but i feel the reason could be perf here? idk. anyway using TORCH_GUARD_OR_FALSE instead of TORCH_GUARD_SIZE_OBLIVIOUS here is appropriate. there is really no clear reason for size oblivious reasoning. or for this logic not to apply when size is not size like size is always >=0 anyway. but bad reasoning can make us not able to infer that although we know its true here. python test/dynamo/test_misc.py -k test_validate_outputs_unbacked Pull Request resolved: #154172 Approved by: https://github.com/bobrenjc93 ghstack dependencies: #154154, #154164, #154167
Stack from ghstack (oldest at bottom):
Fixes #119476
Signed-off-by: Edward Z. Yang [email protected]
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @aakhundov @kadeng