diff --git a/kernels/portable/cpu/util/broadcast_util.cpp b/kernels/portable/cpu/util/broadcast_util.cpp index dcc5a0f8e64..dc74ca5ec3e 100644 --- a/kernels/portable/cpu/util/broadcast_util.cpp +++ b/kernels/portable/cpu/util/broadcast_util.cpp @@ -7,9 +7,9 @@ */ #include -#include #include #include +#include #include namespace torch { @@ -213,10 +213,22 @@ ET_NODISCARD Error get_broadcast_target_size( Tensor::SizesType* out_sizes, const size_t out_sizes_len, size_t* out_dim) { - ET_CHECK_OR_RETURN_ERROR( - tensors_are_broadcastable_between(a_size, b_size), - InvalidArgument, - "Two input tensors should be broadcastable.\n"); + if ET_UNLIKELY (!tensors_are_broadcastable_between(a_size, b_size)) { +#ifdef ET_LOG_ENABLED + const auto a_shape_str = tensor_shape_to_c_string( + executorch::runtime::Span( + a_size.data(), a_size.size())); + const auto b_shape_str = tensor_shape_to_c_string( + executorch::runtime::Span( + b_size.data(), b_size.size())); +#endif + ET_LOG( + Error, + "Two input tensors should be broadcastable but got shapes %s and %s.", + a_shape_str.data(), + b_shape_str.data()); + return executorch::runtime::Error::InvalidArgument; + } auto a_dim = a_size.size(); auto b_dim = b_size.size(); diff --git a/kernels/portable/cpu/util/targets.bzl b/kernels/portable/cpu/util/targets.bzl index d942f7ad9f9..0115feb6256 100644 --- a/kernels/portable/cpu/util/targets.bzl +++ b/kernels/portable/cpu/util/targets.bzl @@ -73,8 +73,8 @@ def define_common_targets(): compiler_flags = ["-Wno-missing-prototypes"], deps = [ ":repeat_util", - ":tensor_util", "//executorch/runtime/kernel:kernel_includes", + "//executorch/runtime/core/exec_aten/util:tensor_shape_to_c_string", "//executorch/runtime/core/exec_aten/util:tensor_util", ], visibility = ["//executorch/kernels/portable/cpu/...", "//executorch/kernels/optimized/cpu/...", "@EXECUTORCH_CLIENTS"], @@ -268,17 +268,6 @@ def define_common_targets(): visibility = ["//executorch/kernels/portable/cpu/..."], ) - runtime.cxx_library( - name = "tensor_util", - srcs = ["tensor_util.cpp"], - exported_headers = ["tensor_util.h"], - deps = [ - "//executorch/runtime/core/exec_aten:lib", - "//executorch/runtime/kernel:kernel_includes", - ], - visibility = ["//executorch/kernels/portable/cpu/..."], - ) - runtime.cxx_library( name = "upsample_util", srcs = ["upsample_util.cpp"], diff --git a/kernels/portable/cpu/util/test/CMakeLists.txt b/kernels/portable/cpu/util/test/CMakeLists.txt index bd18338da2f..5f81e4b6aec 100644 --- a/kernels/portable/cpu/util/test/CMakeLists.txt +++ b/kernels/portable/cpu/util/test/CMakeLists.txt @@ -19,7 +19,7 @@ set(EXECUTORCH_ROOT ${CMAKE_CURRENT_SOURCE_DIR}/../../../../..) include(${EXECUTORCH_ROOT}/build/Test.cmake) -set(_test_srcs broadcast_test.cpp reduce_test.cpp tensor_util_test.cpp) +set(_test_srcs broadcast_test.cpp reduce_test.cpp) et_cxx_test( kernels_portable_cpu_util_test SOURCES ${_test_srcs} EXTRA_LIBS diff --git a/kernels/portable/cpu/util/test/broadcast_test.cpp b/kernels/portable/cpu/util/test/broadcast_test.cpp index 23640f84689..679296f112c 100644 --- a/kernels/portable/cpu/util/test/broadcast_test.cpp +++ b/kernels/portable/cpu/util/test/broadcast_test.cpp @@ -129,6 +129,15 @@ TEST(BroadcastUtilTest, GetBroadcastTargetSize) { EXPECT_TRUE( ArrayRef(expected_output_size, expected_output_dim) .equals(ArrayRef({5, 2, 2}))); + + Tensor c = tf.zeros({4, 5}); + err = get_broadcast_target_size( + a, + c, + expected_output_size, + torch::executor::kTensorDimensionLimit, + &expected_output_dim); + EXPECT_EQ(err, torch::executor::Error::InvalidArgument); } size_t linearize_indexes(size_t* indexes, size_t indexes_len, const Tensor& t) { diff --git a/kernels/portable/cpu/util/test/targets.bzl b/kernels/portable/cpu/util/test/targets.bzl index 0208f3488de..28988b90dcc 100644 --- a/kernels/portable/cpu/util/test/targets.bzl +++ b/kernels/portable/cpu/util/test/targets.bzl @@ -21,11 +21,3 @@ def define_common_targets(): "//executorch/kernels/portable/cpu/util:reduce_util", ], ) - - runtime.cxx_test( - name = "tensor_util_test", - srcs = ["tensor_util_test.cpp"], - deps = [ - "//executorch/kernels/portable/cpu/util:tensor_util", - ], - ) diff --git a/runtime/core/exec_aten/util/dim_order_util.h b/runtime/core/exec_aten/util/dim_order_util.h index a1481ee3871..0aef3e5c6c9 100644 --- a/runtime/core/exec_aten/util/dim_order_util.h +++ b/runtime/core/exec_aten/util/dim_order_util.h @@ -260,35 +260,6 @@ ET_NODISCARD inline Error stride_to_dim_order( return Error::Ok; } -/** - * Print a string representation of an ArrayRef of tensor sizes into a - * user-provided string buffer. If the user buffer is too small, the string - * will be truncated. The output is of the format (1,2,3,4). - * - * Note that we cannot use ArrayRef here due to a circular dependency (see - * above comments). - */ -template -inline void sizes_to_string( - char* output, - size_t output_size, - SizesType* sizes, - size_t rank) { - auto remaining_size = output_size; - for (auto i = 0; remaining_size > 0 && i < rank; i++) { - snprintf( - output, - remaining_size, - "%s%zd", - i == 0 ? "(" : ",", - static_cast(sizes[i])); - auto len = strlen(output); - output += len; - remaining_size -= len; - } - snprintf(output, remaining_size, ")"); -} - } // namespace runtime } // namespace executorch diff --git a/runtime/core/exec_aten/util/targets.bzl b/runtime/core/exec_aten/util/targets.bzl index e66ffa3a028..36e18930121 100644 --- a/runtime/core/exec_aten/util/targets.bzl +++ b/runtime/core/exec_aten/util/targets.bzl @@ -72,3 +72,26 @@ def define_common_targets(): # specify library directory path. force_static = True, ) + + runtime.cxx_library( + name = "tensor_shape_to_c_string" + aten_suffix, + srcs = ["tensor_shape_to_c_string.cpp"], + exported_deps = [ + "//executorch/runtime/core:core", + "//executorch/runtime/core/exec_aten/util:tensor_dimension_limit", + ], + exported_headers = ["tensor_shape_to_c_string.h"], + visibility = [ + "//executorch/...", + "@EXECUTORCH_CLIENTS", + ], + ) + + runtime.cxx_library( + name = "tensor_dimension_limit", + exported_headers = ["tensor_dimension_limit.h"], + visibility = [ + "//executorch/...", + "@EXECUTORCH_CLIENTS", + ], + ) diff --git a/runtime/core/exec_aten/util/tensor_dimension_limit.h b/runtime/core/exec_aten/util/tensor_dimension_limit.h new file mode 100644 index 00000000000..6e072ab0582 --- /dev/null +++ b/runtime/core/exec_aten/util/tensor_dimension_limit.h @@ -0,0 +1,21 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. + */ + +#pragma once + +namespace executorch::runtime { +/** + * The expected output size may not be the existing size of any inputs and + * outputs if the operator supports both broadcast and dynamic shape. + * Therefore such operators needs extra space to store the calculated expected + * output size. such dynamic allocation is troublesome in executorch so we can + * just hard code a static value of a relatively small value because users + * don't create high dimensional tensors. + */ +constexpr size_t kTensorDimensionLimit = 16; +} // namespace executorch::runtime diff --git a/kernels/portable/cpu/util/tensor_util.cpp b/runtime/core/exec_aten/util/tensor_shape_to_c_string.cpp similarity index 69% rename from kernels/portable/cpu/util/tensor_util.cpp rename to runtime/core/exec_aten/util/tensor_shape_to_c_string.cpp index abdb202dfb6..cfd416285c5 100644 --- a/kernels/portable/cpu/util/tensor_util.cpp +++ b/runtime/core/exec_aten/util/tensor_shape_to_c_string.cpp @@ -6,17 +6,19 @@ * LICENSE file in the root directory of this source tree. */ -#include -#include +#include + #include +#include +#include // For snprintf. +#include + namespace executorch::runtime { -/** - * Shared implementation for tensor_util.h, may only contain code that - * works whether or not ATen mode is active. - */ -std::array tensor_shape_to_c_string( - executorch::runtime::Span shape) { +namespace { +template +std::array tensor_shape_to_c_string_impl( + executorch::runtime::Span shape) { std::array out; char* p = out.data(); if ET_UNLIKELY (shape.size() > kTensorDimensionLimit) { @@ -48,5 +50,16 @@ std::array tensor_shape_to_c_string( *(p - 1) = '\0'; return out; } +} // namespace + +std::array tensor_shape_to_c_string( + executorch::runtime::Span shape) { + return tensor_shape_to_c_string_impl(shape); +} + +std::array tensor_shape_to_c_string( + executorch::runtime::Span shape) { + return tensor_shape_to_c_string_impl(shape); +} } // namespace executorch::runtime diff --git a/kernels/portable/cpu/util/tensor_util.h b/runtime/core/exec_aten/util/tensor_shape_to_c_string.h similarity index 58% rename from kernels/portable/cpu/util/tensor_util.h rename to runtime/core/exec_aten/util/tensor_shape_to_c_string.h index e3484f0e118..efc91f8b5d4 100644 --- a/kernels/portable/cpu/util/tensor_util.h +++ b/runtime/core/exec_aten/util/tensor_shape_to_c_string.h @@ -10,10 +10,10 @@ #include #include +#include #include -#include -#include +#include #include namespace executorch::runtime { @@ -34,9 +34,7 @@ constexpr size_t kTensorShapeStringSizeLimit = 1 + /* opening parenthesis */ namespace internal { constexpr size_t kMaximumPrintableTensorShapeElement = - std::is_same_v - ? std::numeric_limits::max() - : std::numeric_limits::max(); + std::numeric_limits::max(); } // namespace internal /** @@ -44,8 +42,27 @@ constexpr size_t kMaximumPrintableTensorShapeElement = * elements of the shape are larger than * kMaximumPrintableTensorShapeElement, those elements will be * rendered as ERR instead. + * + * NOTE: There are two overloads of this function to support both ATen + * tensors and ExecuTorch Tensors, which have different SizesType, + * while also avoiding a dependency on exec_aten.h from this header + * because that would cause a circular dependency. + */ +std::array tensor_shape_to_c_string( + executorch::runtime::Span shape); + +/** + * Convert a shape to a NUL-terminated C string with limited size. If + * elements of the shape are larger than + * kMaximumPrintableTensorShapeElement, those elements will be + * rendered as ERR instead. + * + * NOTE: There are two overloads of this function to support both ATen + * tensors and ExecuTorch Tensors, which have different SizesType, + * while also avoiding a dependency on exec_aten.h from this header + * because that would cause a circular dependency. */ std::array tensor_shape_to_c_string( - executorch::runtime::Span shape); + executorch::runtime::Span shape); } // namespace executorch::runtime diff --git a/runtime/core/exec_aten/util/tensor_util.h b/runtime/core/exec_aten/util/tensor_util.h index 1714bf53747..e16fe63e2a2 100644 --- a/runtime/core/exec_aten/util/tensor_util.h +++ b/runtime/core/exec_aten/util/tensor_util.h @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -893,16 +894,6 @@ inline bool tensor_is_scalar(executorch::aten::Tensor t) { return t.dim() == 0 && t.numel() == 1; } -/** - * The expected output size may not be the existing size of any inputs and - * outputs if the operator supports both broadcast and dynamic shape. - * Therefore such operators needs extra space to store the calculated expected - * output size. such dynamic allocation is troublesome in executorch so we can - * just hard code a static value of a relatively small value because users - * don't create high dimensional tensors. - */ -constexpr size_t kTensorDimensionLimit = 16; - /// Returns the product of dim[0:dim), not including dim. inline size_t getLeadingDims( const executorch::aten::Tensor& tensor, diff --git a/runtime/core/exec_aten/util/test/CMakeLists.txt b/runtime/core/exec_aten/util/test/CMakeLists.txt index 86cdf26e68c..6123827e1ff 100644 --- a/runtime/core/exec_aten/util/test/CMakeLists.txt +++ b/runtime/core/exec_aten/util/test/CMakeLists.txt @@ -19,8 +19,10 @@ set(EXECUTORCH_ROOT ${CMAKE_CURRENT_SOURCE_DIR}/../../../../..) include(${EXECUTORCH_ROOT}/build/Test.cmake) -set(_test_srcs scalar_type_util_test.cpp - operator_impl_example_test.cpp dim_order_util_test.cpp +set(_test_srcs + dim_order_util_test.cpp operator_impl_example_test.cpp + scalar_type_util_test.cpp tensor_shape_to_c_string_test.cpp + tensor_util_test.cpp ) et_cxx_test(runtime_core_exec_aten_util_test SOURCES ${_test_srcs} EXTRA_LIBS) diff --git a/runtime/core/exec_aten/util/test/targets.bzl b/runtime/core/exec_aten/util/test/targets.bzl index 615b7c99a44..7d3114a8f82 100644 --- a/runtime/core/exec_aten/util/test/targets.bzl +++ b/runtime/core/exec_aten/util/test/targets.bzl @@ -46,3 +46,11 @@ def define_common_targets(): "//executorch/runtime/core/exec_aten/util:tensor_util" + aten_suffix, ], ) + + runtime.cxx_test( + name = "tensor_shape_to_c_string_test", + srcs = ["tensor_shape_to_c_string_test.cpp"], + deps = [ + "//executorch/runtime/core/exec_aten/util:tensor_shape_to_c_string", + ], + ) diff --git a/kernels/portable/cpu/util/test/tensor_util_test.cpp b/runtime/core/exec_aten/util/test/tensor_shape_to_c_string_test.cpp similarity index 89% rename from kernels/portable/cpu/util/test/tensor_util_test.cpp rename to runtime/core/exec_aten/util/test/tensor_shape_to_c_string_test.cpp index ee559e60311..8059360afd9 100644 --- a/kernels/portable/cpu/util/test/tensor_util_test.cpp +++ b/runtime/core/exec_aten/util/test/tensor_shape_to_c_string_test.cpp @@ -7,8 +7,8 @@ */ #include -#include #include +#include #include #include @@ -17,7 +17,7 @@ using executorch::runtime::Span; using executorch::runtime::tensor_shape_to_c_string; using executorch::runtime::internal::kMaximumPrintableTensorShapeElement; -TEST(TensorUtilTest, TensorShapeToCStringBasic) { +TEST(TensorShapeToCStringTest, Basic) { std::array sizes = {123, 456, 789}; auto str = tensor_shape_to_c_string( Span(sizes.data(), sizes.size())); @@ -29,7 +29,7 @@ TEST(TensorUtilTest, TensorShapeToCStringBasic) { EXPECT_STREQ(str.data(), "(1234567890)"); } -TEST(TensorUtilTest, TensorShapeToCStringNegativeItems) { +TEST(TensorShapeToCStringTest, NegativeItems) { std::array sizes = {-1, -3, -2, 4}; auto str = tensor_shape_to_c_string( Span(sizes.data(), sizes.size())); @@ -44,7 +44,7 @@ TEST(TensorUtilTest, TensorShapeToCStringNegativeItems) { EXPECT_EQ(str.data(), "(" + std::to_string(one_size[0]) + ")"); } } -TEST(TensorUtilTest, TensorShapeToCStringMaximumElement) { +TEST(TensorShapeToCStringTest, MaximumElement) { std::array sizes = { 123, std::numeric_limits::max(), 789}; auto str = tensor_shape_to_c_string( @@ -60,7 +60,7 @@ TEST(TensorUtilTest, TensorShapeToCStringMaximumElement) { EXPECT_EQ(str.data(), expected_str); } -TEST(TensorUtilTest, TensorShapeToCStringMaximumLength) { +TEST(TensorShapeToCStringTest, MaximumLength) { std::array sizes; std::fill(sizes.begin(), sizes.end(), kMaximumPrintableTensorShapeElement); @@ -78,7 +78,7 @@ TEST(TensorUtilTest, TensorShapeToCStringMaximumLength) { EXPECT_EQ(expected_str, str.data()); } -TEST(TensorUtilTest, TensorShapeToCStringExceedsDimensionLimit) { +TEST(TensorShapeToCStringTest, ExceedsDimensionLimit) { std::array sizes; std::fill(sizes.begin(), sizes.end(), kMaximumPrintableTensorShapeElement); diff --git a/runtime/core/portable_type/targets.bzl b/runtime/core/portable_type/targets.bzl index b8ccbe602ed..dfcd68c96de 100644 --- a/runtime/core/portable_type/targets.bzl +++ b/runtime/core/portable_type/targets.bzl @@ -32,8 +32,9 @@ def define_common_targets(): ":scalar_type", "//executorch/runtime/core:core", "//executorch/runtime/core:tensor_shape_dynamism", - "//executorch/runtime/core/exec_aten/util:scalar_type_util", "//executorch/runtime/core/exec_aten/util:dim_order_util", + "//executorch/runtime/core/exec_aten/util:scalar_type_util", + "//executorch/runtime/core/exec_aten/util:tensor_shape_to_c_string", "//executorch/runtime/core:tag", ], ) diff --git a/runtime/core/portable_type/tensor_impl.cpp b/runtime/core/portable_type/tensor_impl.cpp index 307210a465e..05e65375b66 100644 --- a/runtime/core/portable_type/tensor_impl.cpp +++ b/runtime/core/portable_type/tensor_impl.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -95,18 +96,12 @@ Error TensorImpl::internal_resize_contiguous(ArrayRef new_sizes) { case TensorShapeDynamism::STATIC: if (!std::equal(sizes_, sizes_ + dim_, new_sizes.begin())) { #ifdef ET_LOG_ENABLED - std::array old_sizes_str, new_sizes_str; - - executorch::runtime::sizes_to_string( - old_sizes_str.data(), - old_sizes_str.size(), - sizes().data(), - sizes().size()); - executorch::runtime::sizes_to_string( - new_sizes_str.data(), - new_sizes_str.size(), - new_sizes.data(), - new_sizes.size()); + auto old_sizes_str = executorch::runtime::tensor_shape_to_c_string( + executorch::runtime::Span( + sizes().data(), sizes().size())); + auto new_sizes_str = executorch::runtime::tensor_shape_to_c_string( + executorch::runtime::Span( + new_sizes.data(), new_sizes.size())); #endif ET_CHECK_OR_RETURN_ERROR( diff --git a/test/utils/OSSTestConfig.json b/test/utils/OSSTestConfig.json index ce8c9723d23..2229b255401 100644 --- a/test/utils/OSSTestConfig.json +++ b/test/utils/OSSTestConfig.json @@ -85,10 +85,11 @@ { "directory": "runtime/core/exec_aten/util/test", "sources": [ - "tensor_util_test.cpp", - "scalar_type_util_test.cpp", + "dim_order_util_test.cpp", "operator_impl_example_test.cpp", - "dim_order_util_test.cpp" + "scalar_type_util_test.cpp", + "tensor_shape_to_c_string_test.cpp", + "tensor_util_test.cpp" ] }, {