Preserve caller-provided non-contiguous strides in make_tensor_ptr#19006
Preserve caller-provided non-contiguous strides in make_tensor_ptr#19006manuelcandales wants to merge 1 commit intomainfrom
Conversation
AOTI generates reinterpret_tensor views with non-contiguous strides (e.g. chunk/split for RoPE rotation). Previously make_tensor_ptr asserted that all strides must match contiguous layout, which crashed on these views. Preserve the caller-provided strides when they represent a legitimate non-contiguous view. Also apply the same fix to the duplicate logic in wasm_bindings.cpp, and update tests that expected the old assertion to fire. Co-authored-by: Claude <noreply@anthropic.com>
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/19006
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 New Failures, 2 Cancelled Jobs, 5 Unrelated FailuresAs of commit 76d354f with merge base 401ea8e ( NEW FAILURES - The following jobs have failed:
CANCELLED JOBS - The following jobs were cancelled. Please retry:
FLAKY - The following jobs failed but were likely due to flakiness present on trunk:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
Bot
added
the
CLA Signed
This PR needs a
|
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Allow make_tensor_ptr to accept legitimate non-contiguous stride layouts (e.g., reinterpret_tensor views emitted by AOTI) instead of asserting on non-contiguous strides, and mirror the behavior in the WASM bindings.
Changes:
- Preserve caller-provided non-contiguous strides; only canonicalize to computed strides when the layout is already contiguous (modulo size-1 dims).
- Apply the same stride-preservation logic in
extension/wasm/wasm_bindings.cpp. - Update tensor pointer tests to validate preserved non-contiguous strides instead of expecting assertion deaths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| extension/wasm/wasm_bindings.cpp | Stops forcing computed contiguous strides; preserves caller-provided non-contiguous strides. |
| extension/tensor/tensor_ptr.cpp | Updates make_tensor_ptr to keep caller-provided non-contiguous strides. |
| extension/tensor/test/tensor_ptr_test.cpp | Adjusts expectations to accept non-contiguous strides and removes prior death tests. |
| extension/tensor/test/tensor_ptr_maker_test.cpp | Updates from_blob tests to accept non-contiguous strides. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!strides.empty()) { | ||
| bool is_contiguous = true; | ||
| for (size_t i = 0; i < dim; i++) { | ||
| ET_CHECK_MSG( | ||
| strides[i] == computed_strides[i] || sizes[i] == 1, | ||
| "invalid strides for dim %zu: %" ET_PRI_SIZES_AND_STRIDES | ||
| "!= %" ET_PRI_SIZES_AND_STRIDES | ||
| " while its size is %" ET_PRI_SIZES_AND_STRIDES " != 1", | ||
| i, | ||
| strides[i], | ||
| computed_strides[i], | ||
| sizes[i]); | ||
| if (strides[i] != computed_strides[i] && sizes[i] != 1) { | ||
| is_contiguous = false; | ||
| break; | ||
| } | ||
| } | ||
| if (is_contiguous) { | ||
| strides = std::move(computed_strides); | ||
| } | ||
| // else: keep the caller-provided non-contiguous strides (e.g. from | ||
| // reinterpret_tensor views like chunk/split). | ||
| } else { | ||
| strides = std::move(computed_strides); | ||
| } |
There was a problem hiding this comment.
With this change, make_tensor_ptr can accept dim_order and strides that disagree (i.e., strides not matching dim_order_to_stride even up to size-1 dims). That makes the precedence/contract between dim_order and strides ambiguous and can break any code that assumes dim_order is consistent with the tensor’s stride order. Consider defining an explicit rule, e.g. (mandatory): when caller provides non-empty strides, either (a) derive/normalize dim_order from the provided strides (similar to the existing dim_order.empty() path), or (b) validate that the provided dim_order matches the ordering implied by the provided strides (ignoring size-1 dims) and reject otherwise. Update the tests accordingly (the new TensorMismatchStridesAndDimOrder behavior currently relies on this ambiguity).
| if (!strides.empty()) { | ||
| bool is_contiguous = true; | ||
| for (size_t i = 0; i < dim; i++) { | ||
| ET_CHECK_MSG( | ||
| strides[i] == computed_strides[i] || sizes[i] == 1, | ||
| "invalid strides for dim %zu: %" ET_PRI_SIZES_AND_STRIDES | ||
| "!= %" ET_PRI_SIZES_AND_STRIDES | ||
| " while its size is %" ET_PRI_SIZES_AND_STRIDES " != 1", | ||
| i, | ||
| strides[i], | ||
| computed_strides[i], | ||
| sizes[i]); | ||
| if (strides[i] != computed_strides[i] && sizes[i] != 1) { | ||
| is_contiguous = false; | ||
| break; | ||
| } | ||
| } | ||
| if (is_contiguous) { | ||
| strides = std::move(computed_strides); | ||
| } |
There was a problem hiding this comment.
This removes the only enforcement that strides are a safe/expected layout. As a result, callers can now pass arbitrary strides (including negative/zero strides, or extremely large strides) which can lead to out-of-bounds accesses or integer overflow when computing element offsets/required storage size downstream. At minimum, add validation for obviously unsafe stride patterns (e.g., non-negative strides for non-broadcast dims if negative strides aren’t supported, and overflow-safe computation of the maximum addressable offset for the view). If the API can’t validate against the actual backing allocation size, consider adding an overload that takes the buffer length in bytes (or an explicit storage descriptor) so from_blob/make_tensor_ptr can enforce that the view is within bounds.
| if (!strides.empty()) { | ||
| bool is_contiguous = true; | ||
| for (size_t i = 0; i < sizes.size(); i++) { | ||
| THROW_IF_FALSE( | ||
| strides[i] == computed_strides[i] || sizes[i] == 1, | ||
| "invalid strides for dim %zu: %" ET_PRI_SIZES_AND_STRIDES | ||
| "!= %" ET_PRI_SIZES_AND_STRIDES | ||
| " while its size is %" ET_PRI_SIZES_AND_STRIDES " != 1", | ||
| i, | ||
| strides[i], | ||
| computed_strides[i], | ||
| sizes[i]); | ||
| if (strides[i] != computed_strides[i] && sizes[i] != 1) { | ||
| is_contiguous = false; | ||
| break; | ||
| } | ||
| } | ||
| if (is_contiguous) { | ||
| strides = std::move(computed_strides); | ||
| } | ||
| } else { | ||
| strides = std::move(computed_strides); | ||
| } |
There was a problem hiding this comment.
This duplicates the updated stride canonicalization logic from tensor_ptr.cpp, which risks behavior drift between WASM and non-WASM paths over time. Consider extracting a shared helper (e.g., a small utility that takes sizes, computed_strides, and optional strides and returns the normalized/preserved strides plus any validation) and using it in both places.
| // dim_order={1,0} implies strides={1,3}, but caller provides {1,4}. | ||
| // Non-contiguous strides are preserved for reinterpret_tensor views. |
There was a problem hiding this comment.
The test comment frames this as a reinterpret_tensor-style view, but it constructs a fresh tensor from raw {sizes, dim_order, strides} where dim_order and strides disagree. This is a different scenario than preserving strides from a view and can confuse readers about the intended contract. Suggest clarifying the comment to describe the actual case being tested (mismatched dim_order/strides), and/or adjusting the test to use a reinterpret_tensor-like construction path if that’s the intended coverage.
| // dim_order={1,0} implies strides={1,3}, but caller provides {1,4}. | |
| // Non-contiguous strides are preserved for reinterpret_tensor views. | |
| // This directly constructs a tensor from explicit sizes, dim_order, and | |
| // strides. dim_order={1,0} would normally imply strides={1,3}, but the | |
| // caller provides mismatched strides {1,4}; verify make_tensor_ptr preserves | |
| // the provided strides rather than inferring them from dim_order. |
AOTI generates reinterpret_tensor views with non-contiguous strides (e.g. chunk/split for RoPE rotation). Previously make_tensor_ptr asserted that all strides must match contiguous layout, which crashed on these views. Preserve the caller-provided strides when they represent a legitimate non-contiguous view.
Also apply the same fix to the duplicate logic in wasm_bindings.cpp, and update tests that expected the old assertion to fire.