Decay integer-only (Optional)SymIntArrayRef to IntList in IValue #86094
Conversation
Signed-off-by: Edward Z. Yang <ezyang@fb.com> [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/86094
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: ❌ 1 FailuresAs of commit c00c82c: This comment was automatically generated by Dr. CI and updates every 15 minutes. |
github-actions
bot
requested review from
Chillee,
Krovatkin,
albanD,
anjali411,
antoniojkim,
miladm and
wconstab
aten/src/ATen/core/ivalue_inl.h
Outdated
| if (vi.has_value()) { | ||
| *this = IValue(vi); | ||
| } else { | ||
| *this = IValue(c10::List<c10::SymInt>()); |
There was a problem hiding this comment.
maybe add a comment about why we need the else (vi can be nullopt even if v has one or more elements that are symbolic)
|
This doesn't seem to solve the actual problem |
|
actually I'm wrong, it does seem to work! Not sure if this is the right approach though |
aten/src/ATen/core/ivalue.h
Outdated
| IValue(std::array<T, N> v); | ||
|
|
||
| // TODO: this is not robust | ||
| IValue(at::ArrayRef<c10::SymInt> v); |
There was a problem hiding this comment.
So the reason this is a bit dodgy, is that you might have a std::array<c10::SymInt, 2> or any other container type, and then this overload would not fire. It also seems like we may not be handling OptionalSymIntArrayRef correctly. OTOH, these IValue constructors are the ones where we actually allocate the c10::List, so it's the most convenient place to also test that we've got all ints.
There was a problem hiding this comment.
For the array, IValue(std::array<T, N> v) will be called which always traverses all the elements and thus return a valid IValue (containing SymInt even if it was all int).
Can we just plan to keep specializing the container templates to properly double check the SymInt?
There was a problem hiding this comment.
Yeah, if we want to do it that way, I can add specializations for everything else.
There was a problem hiding this comment.
Oh, just to be clear, it is REQUIRED that we return an IValue that is an IntList if all of the elements happen to be integers only, because downstream code will attempt to toIntList if it is going straight to a kernel that doesn't support symints, and this will choke if you pass it a SymIntList
There was a problem hiding this comment.
Yeah, if we want to do it that way, I can add specializations for everything else.
I'm not sure how much c++ magic can be done here. Are there other options than specializing everything?
There was a problem hiding this comment.
Well, the current code (and I guess the new specializations for other containers) are already quite slow?
You call is_symbolic() on every single element to check if they are symbolic in the asIntArrayRefSlowOpt call.
So if you just modify the list.push_back(SymInt) to check for is_symbolic (and update the tag accordingly), it is the same cost? We do traverse every element in any case.
There was a problem hiding this comment.
Well, the current code (and I guess the new specializations for other containers) are already quite slow?
You call is_symbolic() on every single element to check if they are symbolic in the asIntArrayRefSlowOpt call.
This can be optimized though; we can add a boolean field to SymIntArrayRef to tag if everything is int, then we don't need to do a loopy check.
There was a problem hiding this comment.
So what about:
- Update list.push_back to ensure correctness. It will be as fast as the current version of our code.
- If and when we speed up is_symbolic() checks for all int, we will have to update these overloads to read this new flag (for the ones we care about) and so we can easily opt-into the fast version.
There was a problem hiding this comment.
I think my new plan is to static assert that T is not SymInt on the other containers and move on with my life.
There was a problem hiding this comment.
Note that I'm not saying that we must do either here. If the all symint PR can land, I think we should land this one as is! :)
Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]
ezyang
changed the title
…IValue" Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]
…IValue" We have logic that says if you ask for a SymIntList from an IValue, but the IValue is actually an IntList, we will still give it to you in that case (check ivalue_to_arg in aten/src/ATen/core/boxing/impl/make_boxed_from_unboxed_functor.h). However, we also need the *inverse* version of this logic, which says that if you construct an IValue from a SymIntArrayRef, and it is actually integer only, we need to store it as an IntList, so that toIntList on the IValue will work. The way this works is a bit twisty, but our basic strategy is to disable construction of IValue from list container types that contain SymInt directly, and then directly implement variants of these constructors by hand, which iterate over the elements of the list and test if there are any SymInts or not to decide what type to construct the underlying List. These variants have to be templated, otherwise we will run afoul ambiguous overloads. I only did the overloads that actually occurred in practice; you may need to add more if you SymIntify more stuff. Signed-off-by: Edward Z. Yang <ezyangfb.com> [ghstack-poisoned]
Stack from ghstack (oldest at bottom):
We have logic that says if you ask for a SymIntList from an IValue, but the IValue is actually an IntList, we will still give it to you in that case (check ivalue_to_arg in aten/src/ATen/core/boxing/impl/make_boxed_from_unboxed_functor.h). However, we also need the inverse version of this logic, which says that if you construct an IValue from a SymIntArrayRef, and it is actually integer only, we need to store it as an IntList, so that toIntList on the IValue will work.
The way this works is a bit twisty, but our basic strategy is to disable construction of IValue from list container types that contain SymInt directly, and then directly implement variants of these constructors by hand, which iterate over the elements of the list and test if there are any SymInts or not to decide what type to construct the underlying List. These variants have to be templated, otherwise we will run afoul ambiguous overloads. I only did the overloads that actually occurred in practice; you may need to add more if you SymIntify more stuff.
Signed-off-by: Edward Z. Yang ezyang@fb.com