Reject non-default dim order in the portable kernels that copy by block - #21865
Open
SuryanshSS1011 wants to merge 3 commits into
Open
Reject non-default dim order in the portable kernels that copy by block#21865SuryanshSS1011 wants to merge 3 commits into
SuryanshSS1011 wants to merge 3 commits into
Conversation
SuryanshSS1011
requested review from
kirklandsign and
larryliu0820
as code owners
August 15, 2026 03:09
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21865
Note: Links to docs will display an error until the docs builds have been completed.
|
Contributor
Author
|
@pytorchbot label "release notes: ops & kernels" |
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
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.
Summary
Follow-up to #21828, which fixes the coordinate-indexed half of the same defect. These two PRs touch no files in common and can land in either order.
Five portable kernels accept a channels-last input and return wrong data without erroring. Three more share the defect but are currently masked by an unrelated check. Checked against eager PyTorch with only the memory format differing:
aten.cataten.pixel_unshuffleaten.slice_scatteraten.topkaten.constant_pad_ndaten.cumsumaten.split_copyaten.split_with_sizes_copyThe check that masks the last three is
tensors_have_same_dim_order, and it fires only because the exported graph gives them differing dim orders. Nothing guards the arithmetic itself, so the unit tests below reach those kernels directly.These kernels copy in blocks, taking
getLeadingDimsandgetTrailingDimsaround the operating dim as the length of a contiguous run:That run only exists in the default dim order. Under channels-last the elements after
dimare not adjacent, so the arithmetic walks the wrong bytes.This is the same assumption as #21828, but it needs a different fix. There the kernels index element by element, so reading the tensor's strides is enough. Here the algorithm depends on contiguity itself, and supporting the layout would mean restructuring each loop. So this adds the guard that
op_addmm,op_bmmandop_avg_pool2dalready use:ET_KERNEL_CHECK(ctx, tensor_is_default_dim_order(in), InvalidArgument, out);catneeded it on each input as well as the output, since it had no dim order check at all.pixel_shufflealready carries this exact pair of checks, sopixel_unshufflemissing them looks like an oversight rather than a decision.Note:
catandsplitappear in nearly every model, so a channels-last program that runs today would start failing instead of returning wrong numbers. That still seems better than silent corruption, but happy to restructure the loops to support the layout instead if that's preferred.select_scatterandnative_batch_normuse the same helpers but are not affected. Their auxiliary tensors are lower rank, so the existing same dim order check already forces the input contiguous.unfold_copyis likely affected, but the wrong result I measured came from a multi-op graph and I could not attribute it to the kernel. The same block arithmetic appears in the optimizedlayer_norm, fixed in #21866, and in the quantizeddequantize, which #21517 covers.Test plan
A
NonDefaultDimOrderDiestest per kernel. Each passes a uniformly channels-last set of tensors, so the existing same dim order check passes and only the new guard can reject. Reverting the eight kernel sources while keeping the tests fails all eight.This also adds
op_pixel_unshuffle_test.cpptokernels/test/CMakeLists.txt. The list includesop_pixel_shuffle_test.cppand every other op in that directory but omits this one, so those tests do not run in a CMake build and the new one would not either.