Repository navigation
Index portable kernels by the tensor's strides instead of assuming row-major - #21828
SuryanshSS1011 wants to merge 7 commits into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21828
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: ❌ 2 Cancelled Jobs, 1 Unclassified FailureAs of commit b5ec7d0 with merge base 79e3eae ( UNCLASSIFIED FAILURE - DrCI could not classify the following job because the workflow did not run on the merge base. The failure may be pre-existing on trunk or introduced by this PR:
CANCELLED JOBS - The following jobs were cancelled. Please retry:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@pytorchbot label "release notes: ops & kernels" |
There was a problem hiding this comment.
Pull request overview
This PR addresses silent data corruption in the portable CPU implementations of flip, gather, and scatter when tensors use non-default (e.g., channels-last) dimension order. The approach is to add a default-dim-order guard so these kernels fail fast with InvalidArgument instead of producing incorrect results, and to add regression tests covering the new failure mode.
Changes:
- Added
tensor_is_default_dim_order(...)gating toflip_out,gather_out, andscatter_{src,value}_outportable CPU kernels. - Added regression tests asserting kernel failure on channels-last-like inputs for
flip,gather, andscatter. - Updated
op_flip_test.cppto use anOperatorTestfixture where kernel-failure assertions require access tocontext_.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| kernels/portable/cpu/op_flip.cpp | Adds a default-dim-order guard to prevent incorrect indexing on channels-last tensors. |
| kernels/portable/cpu/op_gather.cpp | Adds a default-dim-order guard for gather_out. |
| kernels/portable/cpu/op_scatter.cpp | Adds a default-dim-order guard for scatter_src_out and scatter_value_out. |
| kernels/test/op_flip_test.cpp | Adds a regression test expecting failure on non-default dim order using an OperatorTest fixture. |
| kernels/test/op_gather_test.cpp | Adds a regression test expecting failure on non-default dim order. |
| kernels/test/op_scatter_test.cpp | Adds a regression test expecting failure on non-default dim order for scatter_src_out. |
Suppressed comments (1)
kernels/portable/cpu/op_scatter.cpp:148
scatter_value_outstill relies onindexToCoordinate(index, ...)andcoordinateToIndex(out, ...)(default-layout indexing). Only checkingtensor_is_default_dim_order(in)can still allow corruption whenindexoroutare non-default. Add checks forindexdefault dim order and requirein/outshare dim order (soindefault impliesoutdefault).
ET_KERNEL_CHECK(ctx, tensor_is_default_dim_order(in), InvalidArgument, out);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (4)
kernels/test/op_scatter_test.cpp:693
NonDefaultDimOrderDiescurrently uses a default-dim-orderindex, so it will fail at the newtensors_have_same_dim_order(in, index, out)check even without thetensor_is_default_dim_order(in)gate. Makeindexchannels-last too so the test asserts rejection of a channels-last input even when all tensors share the same non-default dim order.
Tensor index = tf_index.zeros({1, 1, 2, 2});
kernels/test/op_gather_test.cpp:387
NonDefaultDimOrderDiescurrently fails becauseindexis default dim order whileself/outare channels-last, so it exercises the newtensors_have_same_dim_order(...)check rather than the intendedtensor_is_default_dim_order(in)gate. Makeindexchannels-last too so all tensors share the same non-default dim order and the test asserts the default-dim-order rejection path.
Tensor index = tf_index.zeros({1, 1, 2, 2});
kernels/test/op_scatter_test.cpp:680
NonDefaultDimOrderDiescurrently fails due to mixed dim orders (index/srcare default whileself/outare channels-last), so it doesn't validate the newtensor_is_default_dim_order(in)guard. Use channels-lastindex/srcso all tensors share the same non-default dim order and the test specifically covers the default-dim-order rejection.
This issue also appears on line 693 of the same file.
Tensor index = tf_index.zeros({1, 1, 2, 2});
Tensor src = tf_data.ones({1, 1, 2, 2});
kernels/portable/cpu/op_scatter_add.cpp:74
- This PR also adds a default-dim-order rejection to
scatter_add_out(and adds new non-default-dim-order tests forroll_out/permute_copy_out), but the PR title/summary/test plan only call outflip,gather, andscatter. Consider updating the PR metadata to reflect the expanded scope so downstream users aren't surprised by additional operators rejecting channels-last inputs.
ET_KERNEL_CHECK(
ctx, tensors_have_same_dim_order(self, src, out), InvalidArgument, out);
ET_KERNEL_CHECK(
ctx, tensor_is_default_dim_order(index), InvalidArgument, out);
ET_KERNEL_CHECK(ctx, tensor_is_default_dim_order(self), InvalidArgument, out);
af91b42 to
035e6b6
Compare
|
Seems these changes impact some cadence kernels that use the helper, cc @mcremon-meta executorch/backends/cadence/hifi/operators/op_permute_copy.cpp Lines 143 to 151 in b5ec7d0 |
thanks for the heads up, we don't use dim order so everything should always be contiguous and we should be fine. If CI doesn't break (including the internal Meta CI, which could trigger a revert of this), no objection from my side. |
…21866) ### Summary Related to #21828 and #21865 (same underlying assumption in the optimized kernel library). `aten.native_layer_norm` returns wrong data on a channels-last input, off by 3.192 against eager PyTorch where it matches exactly on contiguous input. Nothing errors. The portable kernel is fine. It carries this, with the reason written down: ```cpp // Only support default dim order for now. // TODO: Support other dim orders. ET_KERNEL_CHECK( ctx, tensor_is_default_dim_order(input), InvalidArgument, ret_val); ``` The optimized kernel has neither that check nor the matching `tensors_have_same_dim_order`, and `native_layer_norm.out` is mapped to `torch::executor::opt_native_layer_norm_out` in `optimized.yaml`. So any build with `EXECUTORCH_BUILD_KERNELS_OPTIMIZED=ON` gets the ungated one. It needs the guard rather than stride-aware indexing, because it splits the buffer into `M` rows of `N` contiguous elements: ```cpp const size_t M = getLeadingDims(input, dim); const size_t N = getTrailingDims(input, dim) * dim_size; ``` That layout only exists in the default dim order. This copies the two checks across from the portable kernel so the two agree. ### Test plan `OpNativeLayerNormTest.NonDefaultDimOrderDies` passes a channels-last input with `out`, `mean` and `rstd` all channels-last, so the same dim order check passes and only the default dim order check can reject. `mean` and `rstd` share the input's rank with the normalized dims set to 1, which the kernel requires before it reaches any dim order check. The test file is shared by both kernel libraries, so it runs against the portable kernel too, where it already passes.
@mcremon-meta could you check again now that CI looks good? |
…ck (#21865) ### 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: | kernel | contiguous input | channels-last input | |---|---|---| | `aten.cat` | matches | wrong, max abs diff 4.141 | | `aten.pixel_unshuffle` | matches | wrong, max abs diff 3.941 | | `aten.slice_scatter` | matches | wrong, max abs diff 3.376 | | `aten.topk` | matches | wrong, max abs diff 2.581 | | `aten.constant_pad_nd` | matches | wrong, max abs diff 2.526 | | `aten.cumsum` | matches | rejected, status 0x12 | | `aten.split_copy` | matches | rejected, status 0x12 | | `aten.split_with_sizes_copy` | matches | rejected, status 0x12 | The 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 `getLeadingDims` and `getTrailingDims` around the operating dim as the length of a contiguous run: ```cpp const size_t outer = getLeadingDims(out, dim); const size_t dim_stride = getTrailingDims(out, dim); ``` That run only exists in the default dim order. Under channels-last the elements after `dim` are 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_bmm` and `op_avg_pool2d` already use: ```cpp ET_KERNEL_CHECK(ctx, tensor_is_default_dim_order(in), InvalidArgument, out); ``` `cat` needed it on each input as well as the output, since it had no dim order check at all. `slice_scatter` needed `src` in its same dim order check as well, since the kernel reads `src` as one contiguous run and only `input` and `out` were covered. `pixel_shuffle` already carries this exact pair of checks, so `pixel_unshuffle` missing them looks like an oversight rather than a decision. Note: `cat` and `split` appear 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. `native_batch_norm` uses the same helpers but is not affected, since it checks `is_contiguous_dim_order` on the input directly. `select_scatter` and `unfold_copy` are affected and are left to a follow-up. `select_scatter` carries only the same dim order check, which passes when a rank-5 input and a rank-4 `src` are both channels-last, because `is_channels_last_dim_order` accepts 4 and 5 dims. `unfold_copy` has no dim order check at all. The same block arithmetic appears in the optimized `layer_norm`, fixed in #21866, and in the quantized `dequantize`, which #21517 covers. ### Test plan A `NonDefaultDimOrderDies` test 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.cpp` to `kernels/test/CMakeLists.txt`. It is registered in `targets.bzl` but missing from the CMake list, so those tests do not run in a CMake build and the new one would not either. Ten other op tests are missing the same way, and I have left those alone to keep this to one change. cc @larryliu0820 @manuelcandales @JakeStevens
Summary
Three portable kernels accept a channels-last input, return a tensor correctly labelled channels-last, and fill it with wrong data. Nothing errors. Three more share the defect but are currently masked by an unrelated check. Verified all against eager PyTorch with the same op and values and only the memory format differing:
aten.flipaten.gatheraten.scatteraten.scatter_addaten.rollaten.permute_copyThe cause is one line in
coordinateToIndex(runtime/core/exec_aten/util/tensor_util.h):index += coordinate[d] * getTrailingDims(tensor, d);getTrailingDimsis the product of the sizes afterd, which is the row-major stride. A(2, 3, 4, 4)channels-last tensor has strides[48, 1, 12, 3], but that gives[48, 16, 4, 1], so the buffer is walked in the wrong order.This reads
tensor.strides()instead, andmemoizeTrailingDimslikewise for its one caller,permute_copy. For a contiguous tensor the two are equal by definition, so that path is unchanged. It is also cheaper, sincegetTrailingDimsrecomputes a product with an overflow check per dimension.That alone fixes
gather,scatterandscatter_add.flip,rollandpermute_copyalso wrote their output linearly by flat index, which is only correct when the output is contiguous, so those writes are mapped too. The guard onscatter_add'sindexexisted only because this indexing was row-major, so it goes.getTrailingDimsitself is unchanged. Elsewhere it means the length of a contiguous run, for the block copies incat,splitand others, which share the defect but need a different fix.I took this over adding a
tensor_is_default_dim_orderguard to each kernel. A guard would stop the corruption, but the same assumption appears in a dozen more places, and #16429 asks for non-contiguous dim order to be supported rather than rejected. Happy to switch if that's preferred.Test plan
This PR adds seven tests, one per kernel. Each runs a channels-last input and checks it against the result the same values produce contiguously, so it passes only if the kernel honors the tensor's dim order. All seven fail before this change.
cc @larryliu0820 @manuelcandales @JakeStevens