Back out "Revert D25903846: [pytorch][PR] Structured kernel definition for upsample_nearest2d"#50794
Closed
ezyang wants to merge 2 commits intogh/ezyang/900/basefrom
Closed
Back out "Revert D25903846: [pytorch][PR] Structured kernel definition for upsample_nearest2d"#50794ezyang wants to merge 2 commits intogh/ezyang/900/basefrom
ezyang wants to merge 2 commits intogh/ezyang/900/basefrom
Conversation
…n for upsample_nearest2d" Original commit changeset: b4a7948088c0 There are some subtle extra tweaks on top of the original: * There's a bugfix in the codegen to test if a dispatch key is structured *before* short circuiting because the dispatch key was missing in the table. This accounts for mixed structured-nonstructured situations where the dispatch table is present, but the relevant structured key isn't (because the dispatch table only exists to register, e.g., QuantizedCPU) * Dispatch tables for structured kernels are explicitly zeroed, to prevent Math entries from being generated for them. Ideally this logic would happen in the code itself but I tried a simple version of this and it didn't work, so I'm going to do this workaround first and investigate more. * Finally, unlike the original PR, I switched the .vec variant of upsample_nearest2d to also be DefaultBackend, bringing it inline with upsample_nearest1d. Differential Revision: [D25962873](https://our.internmc.facebook.com/intern/diff/D25962873/) [ghstack-poisoned]
ezyang
added a commit
that referenced
this pull request
Jan 20, 2021
…n for upsample_nearest2d" Original commit changeset: b4a7948088c0 There are some subtle extra tweaks on top of the original: * There's a bugfix in the codegen to test if a dispatch key is structured *before* short circuiting because the dispatch key was missing in the table. This accounts for mixed structured-nonstructured situations where the dispatch table is present, but the relevant structured key isn't (because the dispatch table only exists to register, e.g., QuantizedCPU) * Dispatch tables for structured kernels are explicitly zeroed, to prevent Math entries from being generated for them. Ideally this logic would happen in the code itself but I tried a simple version of this and it didn't work, so I'm going to do this workaround first and investigate more. * Finally, unlike the original PR, I switched the .vec variant of upsample_nearest2d to also be DefaultBackend, bringing it inline with upsample_nearest1d. Differential Revision: [D25962873](https://our.internmc.facebook.com/intern/diff/D25962873/) ghstack-source-id: 120018465 Pull Request resolved: #50794
…l definition for upsample_nearest2d"" Original commit changeset: b4a7948088c0 There are some subtle extra tweaks on top of the original. I can unbundle them, but I've opted to keep it with the port because it's the easiest way to make sure the changes are exercised. * There's a bugfix in the codegen to test if a dispatch key is structured *before* short circuiting because the dispatch key was missing in the table. This accounts for mixed structured-nonstructured situations where the dispatch table is present, but the relevant structured key isn't (because the dispatch table only exists to register, e.g., QuantizedCPU) * Dispatch tables for functions which delegate to structured kernels don't have Math entries from generated for them. * It's now illegal to specify a structured dispatch key in a delegated structured kernel (it will be ignored!) add is now fixed to follow this * There are some extra sanity checks for NativeFunctions validation * Finally, unlike the original PR, I switched the .vec variant of upsample_nearest2d to also be DefaultBackend, bringing it inline with upsample_nearest1d. Differential Revision: [D25962873](https://our.internmc.facebook.com/intern/diff/D25962873/) Differential Revision: [D25962873](https://our.internmc.facebook.com/intern/diff/D25962873) [ghstack-poisoned]
ezyang
added a commit
that referenced
this pull request
Jan 20, 2021
…n for upsample_nearest2d" Pull Request resolved: #50794 Original commit changeset: b4a7948088c0 There are some subtle extra tweaks on top of the original. I can unbundle them, but I've opted to keep it with the port because it's the easiest way to make sure the changes are exercised. * There's a bugfix in the codegen to test if a dispatch key is structured *before* short circuiting because the dispatch key was missing in the table. This accounts for mixed structured-nonstructured situations where the dispatch table is present, but the relevant structured key isn't (because the dispatch table only exists to register, e.g., QuantizedCPU) * Dispatch tables for functions which delegate to structured kernels don't have Math entries from generated for them. * It's now illegal to specify a structured dispatch key in a delegated structured kernel (it will be ignored!) add is now fixed to follow this * There are some extra sanity checks for NativeFunctions validation * Finally, unlike the original PR, I switched the .vec variant of upsample_nearest2d to also be DefaultBackend, bringing it inline with upsample_nearest1d. ghstack-source-id: 120038038 Differential Revision: [D25962873](https://our.internmc.facebook.com/intern/diff/D25962873/)
soulitzer
approved these changes
Jan 20, 2021
Contributor
There was a problem hiding this comment.
LGTM. Thanks for debugging this. I didn't realize that all the calls I made while trying to replicate this were actually going to the vec overload, so this cleared it up.
Some noob questions -
This accounts for mixed structured-nonstructured situations where the dispatch table is present, but the relevant structured key isn't (because the dispatch table only exists to register, e.g., QuantizedCPU)
- Whats a situation when the dispatch table is not present?
- What do you mean by relevant structured key? It seems that if input is quantized for example, neither structured key is particularly relevant
- Is there a reason why we only check the out variant to see if there is a matching dispatch before short circuiting. Seems like checking non-out variants would fix the issue as well here
Contributor
Author
Specifically referring to when you don't have a
Sorry, this wasn't very clear. The bug in question was when you were processing QuantizedCPU for the functional version of an operator, and that dispatch key didn't exist for the out version. If you first test if QuantizedCPU exists in the out version, you'll conclude there is nothing to do; but actually, you needed to fallback to unstructured generator case where there is something to do for the functional operator only.
For structured dispatch keys (CPU and CUDA), you are guaranteed to not have keys on anything besides the out variant.! |
Contributor
|
This pull request has been merged in 5e79b8e. |
laurentdupin
pushed a commit
to laurentdupin/pytorch
that referenced
this pull request
Apr 24, 2026
…n for upsample_nearest2d" (pytorch#50794) Summary: Pull Request resolved: pytorch#50794 Original commit changeset: b4a7948088c0 There are some subtle extra tweaks on top of the original. I can unbundle them, but I've opted to keep it with the port because it's the easiest way to make sure the changes are exercised. * There's a bugfix in the codegen to test if a dispatch key is structured *before* short circuiting because the dispatch key was missing in the table. This accounts for mixed structured-nonstructured situations where the dispatch table is present, but the relevant structured key isn't (because the dispatch table only exists to register, e.g., QuantizedCPU) * Dispatch tables for functions which delegate to structured kernels don't have Math entries from generated for them. * It's now illegal to specify a structured dispatch key in a delegated structured kernel (it will be ignored!) add is now fixed to follow this * There are some extra sanity checks for NativeFunctions validation * Finally, unlike the original PR, I switched the .vec variant of upsample_nearest2d to also be DefaultBackend, bringing it inline with upsample_nearest1d. ghstack-source-id: 120038038 Test Plan: ``` buck test mode/dev //coreai/tiefenrausch:python_tests -- --exact 'coreai/tiefenrausch:python_tests - test_can_run_local_async_inference_cpu (coreai.tiefenrausch.tests.python_test.TiefenrauschPY)' --run-disabled ``` Reviewed By: ngimel Differential Revision: D25962873 fbshipit-source-id: d29a9c97f15151db3066ae5efe7a0701e6dc05a3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
Original commit changeset: b4a7948088c0
There are some subtle extra tweaks on top of the original. I can unbundle them, but I've opted to keep it with the port because it's the easiest way to make sure the changes are exercised.
Differential Revision: D25962873
Differential Revision: D25962873