From 404f1d536200fe4de716ba0e736ac2f7000d76ab Mon Sep 17 00:00:00 2001 From: Scott Roy Date: Wed, 25 Feb 2026 12:58:54 -0800 Subject: [PATCH] Remove asserts in CoreML resize (#17626) Summary: Pull Request resolved: https://github.com/pytorch/executorch/pull/17626 The `assert(shape[i] <= shape_[i])` constraint in `MultiArray::MemoryLayout::resize()` was unnecessary and overly restrictive. `MemoryLayout` is purely metadata describing shape and strides - it has no knowledge of the underlying memory allocation. The actual memory safety is guaranteed at a higher level: ExecuTorch tensors are pre-allocated with sufficient capacity to handle dynamic output shapes from CoreML model execution. The resize function is called in `ETCoreMLModelManager.mm` to update output tensor metadata after model inference, where the new shape comes directly from CoreML's `modelOutputs[i].shape`. Since the ExecuTorch output buffers are sized appropriately for the model's maximum output dimensions, the resize operation is always safe regardless of whether the new shape is smaller or larger than the previous metadata indicated. The remaining asserts are retained because they catch actual programming errors: - `assert(shape.size() == shape_.size())` - prevents rank mismatch causing out-of-bounds access - `assert(shape[i] >= 1)` - ensures valid dimensions for correct stride computation Reviewed By: shabbyowen, GregoryComer Differential Revision: D93942192 --- .../coreml/runtime/delegate/multiarray.h | 2 +- .../coreml/runtime/delegate/multiarray.mm | 101 +++++++++--------- 2 files changed, 50 insertions(+), 53 deletions(-) diff --git a/backends/apple/coreml/runtime/delegate/multiarray.h b/backends/apple/coreml/runtime/delegate/multiarray.h index ecde904409b..429fe37f919 100644 --- a/backends/apple/coreml/runtime/delegate/multiarray.h +++ b/backends/apple/coreml/runtime/delegate/multiarray.h @@ -85,7 +85,7 @@ class MultiArray final { bool is_packed() const noexcept; // Resizes memory layout - // New shape must be the same dimension and no larger than current shape in all dimensions + // New shape must be the same rank as the current shape // New format is contiguous void resize(const std::vector& shape); diff --git a/backends/apple/coreml/runtime/delegate/multiarray.mm b/backends/apple/coreml/runtime/delegate/multiarray.mm index 6e8efc66339..c390af9c0b4 100644 --- a/backends/apple/coreml/runtime/delegate/multiarray.mm +++ b/backends/apple/coreml/runtime/delegate/multiarray.mm @@ -37,7 +37,7 @@ if (rank > BNNS_MAX_TENSOR_DIMENSION) { return std::nullopt; } - + if (std::is_sorted(multi_array_strides.begin(), multi_array_strides.end(), std::less())) { first_major = false; std::copy(multi_array_strides.begin(), multi_array_strides.end(), bnns_strides); @@ -47,7 +47,7 @@ } else { return std::nullopt; } - + // See BNNSDataLayout's raw value how this bitwise-or makes sense. return (BNNSDataLayout) (0x08000 + // flags as canonical first/last major type 0x10000 * rank + // set dimensionality @@ -100,18 +100,18 @@ bool init_bnns_descriptor(BNNSNDArrayDescriptor& bnns_descriptor, const MultiArr if (layout.num_elements() == 1) { return false; } - + auto bnns_datatype = get_bnns_data_type(layout.dataType()); if (!bnns_datatype) { return false; } - + std::memset(&bnns_descriptor, 0, sizeof(bnns_descriptor)); auto bnns_layout = get_bnns_data_layout(layout.strides(), bnns_descriptor.stride); if (!bnns_layout) { return false; } - + const auto& shape = layout.shape(); std::copy(shape.begin(), shape.end(), bnns_descriptor.size); bnns_descriptor.layout = bnns_layout.value(); @@ -119,7 +119,7 @@ bool init_bnns_descriptor(BNNSNDArrayDescriptor& bnns_descriptor, const MultiArr bnns_descriptor.data_bias = 0.0f; bnns_descriptor.data_type = bnns_datatype.value(); bnns_descriptor.data = multi_array.data(); - + return true; } @@ -137,34 +137,34 @@ bool copy_using_bnns(const MultiArray& src, MultiArray& dst) { if (!init_bnns_descriptor(src_descriptor, src)) { return false; } - + BNNSNDArrayDescriptor dst_descriptor; if (!init_bnns_descriptor(dst_descriptor, dst)) { return false; } - + return BNNSCopy(&dst_descriptor, &src_descriptor, NULL) == 0; } std::vector get_layouts(const std::vector& arrays) { std::vector result; result.reserve(arrays.size()); - + std::transform(arrays.begin(), arrays.end(), std::back_inserter(result), [](const auto& array) { return array.layout(); }); - + return result; } std::vector get_datas(const std::vector& arrays) { std::vector result; result.reserve(arrays.size()); - + std::transform(arrays.begin(), arrays.end(), std::back_inserter(result), [](const auto& array) { return array.data(); }); - + return result; } @@ -178,7 +178,7 @@ bool can_coalesce_dimensions(const std::vector& shape, if (shape1 == 1 || shape2 == 1) { return true; } - + auto stride1 = strides[dim1]; auto stride2 = strides[dim2]; return shape1 * stride1 == stride2; @@ -193,7 +193,7 @@ bool can_coalesce_dimensions(const std::vector& shape, return false; } } - + return true; } @@ -209,7 +209,7 @@ void update_strides(std::vector>& all_strides, if (layouts.size() == 0) { return {}; } - + std::vector shape = layouts.back().shape(); // reverse shape. std::reverse(shape.begin(), shape.end()); @@ -237,16 +237,16 @@ void update_strides(std::vector>& all_strides, } } } - + if (rank == prev_dim + 1) { return layouts; } - + shape.resize(prev_dim + 1); for (auto& strides : all_strides) { strides.resize(prev_dim + 1); } - + std::vector result; result.reserve(layouts.size()); std::reverse(shape.begin(), shape.end()); @@ -254,7 +254,7 @@ void update_strides(std::vector>& all_strides, std::reverse(all_strides[i].begin(), all_strides[i].end()); result.emplace_back(layouts[i].dataType(), shape, std::move(all_strides[i])); } - + return result; } @@ -304,10 +304,10 @@ void decrement_data_pointers(std::vector& data_pointers, class MultiArrayIterator final { public: explicit MultiArrayIterator(const std::vector& arrays) - :datas_(get_datas(arrays)), + :datas_(get_datas(arrays)), layouts_(coalesce_dimensions(get_layouts(arrays))) {} - + private: template void exec(FN&& fn, const std::vector& layouts, std::vector datas, size_t n) { @@ -335,10 +335,10 @@ void exec(FN&& fn, const std::vector& layouts, std::ve } ::decrement_data_pointers(datas, i, 1, layouts); } - + break; } - + default: { const size_t bound = layouts.back().shape()[n - 1]; for (size_t index = 0; index < bound; ++index) { @@ -349,14 +349,14 @@ void exec(FN&& fn, const std::vector& layouts, std::ve } } } - + public: template void exec(FN&& fn) { std::vector datas = datas_; exec(fn, layouts_, datas, layouts_[0].rank()); } - + private: std::vector datas_; std::vector layouts_; @@ -379,42 +379,42 @@ void copy(void *dst, ::copy_value(dst, src); break; } - + case MultiArray::DataType::Byte: { ::copy_value(dst, src); break; } - + case MultiArray::DataType::Char: { ::copy_value(dst, src); break; } - + case MultiArray::DataType::Short: { ::copy_value(dst, src); break; } - + case MultiArray::DataType::Int32: { ::copy_value(dst, src); break; } - + case MultiArray::DataType::Int64: { ::copy_value(dst, src); break; } - + case MultiArray::DataType::Float16: { ::copy_value<_Float16, T>(dst, src); break; } - + case MultiArray::DataType::Float32: { ::copy_value(dst, src); break; } - + case MultiArray::DataType::Float64: { ::copy_value(dst, src); break; @@ -431,42 +431,42 @@ void copy(void *dst, ::copy(dst, dst_data_type, src); break; } - + case MultiArray::DataType::Byte: { ::copy(dst, dst_data_type, src); break; } - + case MultiArray::DataType::Char: { ::copy(dst, dst_data_type, src); break; } - + case MultiArray::DataType::Short: { ::copy(dst, dst_data_type, src); break; } - + case MultiArray::DataType::Int32: { ::copy(dst, dst_data_type, src); break; } - + case MultiArray::DataType::Int64: { ::copy(dst, dst_data_type, src); break; } - + case MultiArray::DataType::Float16: { ::copy<_Float16>(dst, dst_data_type, src); break; } - + case MultiArray::DataType::Float32: { ::copy(dst, dst_data_type, src); break; } - + case MultiArray::DataType::Float64: { ::copy(dst, dst_data_type, src); break; @@ -478,7 +478,7 @@ void copy(const MultiArray& src, MultiArray& dst, MultiArray::CopyOptions option if (options.use_bnns && copy_using_bnns(src, dst)) { return; } - + if (options.use_memcpy && src.layout().dataType() == dst.layout().dataType() && src.layout().is_packed() && @@ -486,7 +486,7 @@ void copy(const MultiArray& src, MultiArray& dst, MultiArray::CopyOptions option std::memcpy(dst.data(), src.data(), src.layout().num_elements() * src.layout().num_bytes()); return; } - + auto iterator = MultiArrayIterator({src, dst}); iterator.exec([&](const std::vector& datas){ void *src_data = datas[0]; @@ -500,7 +500,7 @@ ssize_t get_data_offset(const std::vector& indices, const std::vector(indices[i]) * strides[i]; } - + return offset; } @@ -513,7 +513,7 @@ ssize_t get_data_offset(size_t index, const std::vector& shape, const st offset += dim_index * strides[i]; index %= div; } - + return offset; } } @@ -522,12 +522,9 @@ ssize_t get_data_offset(size_t index, const std::vector& shape, const st void MultiArray::MemoryLayout::resize(const std::vector& shape) { assert(shape.size() == shape_.size()); - for (int i = 0; i < shape.size(); ++i) { - assert (shape[i] >= 1); - assert(shape[i] <= shape_[i]); - } int stride = 1; for (int i = shape.size() - 1; i >= 0; --i) { + assert(shape[i] >= 1); shape_[i] = shape[i]; strides_[i] = stride; if (shape[i] > 1) { @@ -542,7 +539,7 @@ ssize_t get_data_offset(size_t index, const std::vector& shape, const st if (shape_.size() == 0) { return 0; } - + return std::accumulate(shape_.begin(), shape_.end(), size_t(1), std::multiplies()); } @@ -550,7 +547,7 @@ ssize_t get_data_offset(size_t index, const std::vector& shape, const st if (strides_.size() < 2) { return true; } - + ssize_t expectedStride = 1; auto stridesIt = strides_.crbegin(); for (auto shapeIt = shape_.crbegin(); shapeIt!= shape_.crend(); shapeIt++) { @@ -560,7 +557,7 @@ ssize_t get_data_offset(size_t index, const std::vector& shape, const st expectedStride = expectedStride * (*shapeIt); stridesIt++; } - + return true; }