Conversation
Signed-off-by: Edward Z. Yang <ezyang@fb.com>
|
@JackCaoG lets see if this'd do it |
ce170d1 to
86f003e
Compare
86f003e to
e43f4d3
Compare
|
Looks like it worked. So I can land this companion diff instead of mine when we finally land. |
07595d3 to
e43f4d3
Compare
| // `opName_symint` counters. When we finish migrating the ops to symints, we | ||
| // would remove this logic and fix all the tests | ||
| auto changed_symint = | ||
| start_msnap_->CounterChanged(counter_regex, *end_msnap_, ignore_set); |
There was a problem hiding this comment.
Why is this line not saying counter_regex + "_symint"? I am trying to understand the diff between line 54 and 63 here.
There was a problem hiding this comment.
seems likes a typo we should fix
|
|
||
| at::Tensor XLANativeFunctions::view(const at::Tensor& self, | ||
| at::SymIntArrayRef sym_size) { | ||
| at::Tensor XLANativeFunctions::view_symint(const at::Tensor& self, |
There was a problem hiding this comment.
To catch up:
- I understand this PR only enables
SymIntforview - I don't expect
viewis ready for Dynamic Shape through this change - i.e. more work would be needed for DS enablement. The same applies toempty,narrow_copy.
@Krovatkin @JackCaoG please confirm (or correct me if inaccurate).
There was a problem hiding this comment.
Yeah, all this does is change the calling convention; but the insides still assert that the int is not symbolic. Once the IR side is ready, you can just modify this function to track it.
Future SymInt'ified schemas will not get updates like this, because we subsequently added an interlock (the symint: field in the yaml) which controls which calling convention you get. But there's a few stragglers like this from before we had this interlock.
There was a problem hiding this comment.
Once the IR side is ready, you can just modify this function to track it.
Is this part of an existing PR already? If so, please point me to it so I follow your changes accordingly.
There was a problem hiding this comment.
no, that was up to you guys :)
Make ExpectCounterChanged to account for _symint ops without fiMake ExpectCounterChanged to account for _symint ops without fixing the tests or ops explicitlyxing the tests or ops explicitly.
we are hitting test failures w/ counters after we symintified a few kernel and added the "_symint" suffix. Unfortunately, the failures are bit cryptic and requires some investigation: https://github.com/pytorch/pytorch/runs/8231927032?check_suite_focus=true . These failures would happen every time we rename an op.
This PR tweaks
ExpectCounterChangedto account for possible renames of ops to_symint.