Index portable kernels by the tensor's strides instead of assuming row-major

[SuryanshSS1011]

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:

kernel	contiguous input	channels-last input
aten.flip	matches	wrong, max abs diff 32
aten.gather	matches	wrong, max abs diff 30
aten.scatter	matches	wrong, max abs diff 30
aten.scatter_add	matches	rejected, status 0x12
aten.roll	matches	rejected, status 0x12
aten.permute_copy	matches	rejected, status 0x12

The cause is one line in coordinateToIndex (runtime/core/exec_aten/util/tensor_util.h):

index += coordinate[d] * getTrailingDims(tensor, d);

getTrailingDims is the product of the sizes after d, 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, and memoizeTrailingDims likewise 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, since getTrailingDims recomputes a product with an overflow check per dimension.

That alone fixes gather, scatter and scatter_add. flip, roll and permute_copy also wrote their output linearly by flat index, which is only correct when the output is contiguous, so those writes are mapped too. The guard on scatter_add's index existed only because this indexing was row-major, so it goes.

getTrailingDims itself is unchanged. Elsewhere it means the length of a contiguous run, for the block copies in cat, split and others, which share the defect but need a different fix.

I took this over adding a tensor_is_default_dim_order guard 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.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21828

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

⚠️ 13 Awaiting Approval

As of commit a8b5c9b with merge base 79e3eae ():

AWAITING APPROVAL - The following workflows need approval before CI can run:

• Build documentation (gh)
• Build Presets (gh)
• Cadence Build & Test (gh)
• Lint (gh)
• periodic (gh)
• pull (gh)
• Test ARM Backend (gh)
• Test CoreML Backend (gh)
• Test QNN Backend (gh)
• Test QNN Windows MSVC build (gh)
• Test Vulkan Backend (gh)
• Test WebGPU Backend (gh)
• Test XNNPACK Backend (gh)

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[SuryanshSS1011]

@pytorchbot label "release notes: ops & kernels"

[Copilot]

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 to flip_out, gather_out, and scatter_{src,value}_out portable CPU kernels.
• Added regression tests asserting kernel failure on channels-last-like inputs for flip, gather, and scatter.
• Updated op_flip_test.cpp to use an OperatorTest fixture where kernel-failure assertions require access to context_.

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_out still relies on indexToCoordinate(index, ...) and coordinateToIndex(out, ...) (default-layout indexing). Only checking tensor_is_default_dim_order(in) can still allow corruption when index or out are non-default. Add checks for index default dim order and require in/out share dim order (so in default implies out default).

  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.

[Copilot]

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

• NonDefaultDimOrderDies currently uses a default-dim-order index, so it will fail at the new tensors_have_same_dim_order(in, index, out) check even without the tensor_is_default_dim_order(in) gate. Make index channels-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

• NonDefaultDimOrderDies currently fails because index is default dim order while self/out are channels-last, so it exercises the new tensors_have_same_dim_order(...) check rather than the intended tensor_is_default_dim_order(in) gate. Make index channels-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

• NonDefaultDimOrderDies currently fails due to mixed dim orders (index/src are default while self/out are channels-last), so it doesn't validate the new tensor_is_default_dim_order(in) guard. Use channels-last index/src so 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 for roll_out / permute_copy_out), but the PR title/summary/test plan only call out flip, gather, and scatter. 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);

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.