Move tensor_shape_to_c_string back to runtime/core

I moved it to kernels/portable due to build issues, but it doesn't make sense that kernels/portable code can't use this if it's in runtime/core, so let's dig in.

ghstack-source-id: 0b97f31
ghstack-comment-id: 2641470188
Pull Request resolved: #8296

Author: swolchok

kernels/portable/cpu/util/broadcast_util.cpp

@@ -7,9 +7,9 @@
  */
 
 #include <executorch/kernels/portable/cpu/util/repeat_util.h>
-#include <executorch/kernels/portable/cpu/util/tensor_util.h>
 #include <executorch/runtime/core/exec_aten/exec_aten.h>
 #include <executorch/runtime/core/exec_aten/util/scalar_type_util.h>
+#include <executorch/runtime/core/exec_aten/util/tensor_shape_to_c_string.h>
 #include <string.h>
 
 namespace torch {

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"],

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

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",
-        ],
-    )

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 <class SizesType>
-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<size_t>(sizes[i]));
-    auto len = strlen(output);
-    output += len;
-    remaining_size -= len;
-  }
-  snprintf(output, remaining_size, ")");
-}
-
 } // namespace runtime
 } // namespace executorch
 

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",
+        ],
+    )

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

kernels/portable/cpu/util/tensor_util.cpp renamed to 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 <executorch/kernels/portable/cpu/util/tensor_util.h>
-#include <executorch/runtime/core/exec_aten/util/tensor_util.h>
+#include <executorch/runtime/core/exec_aten/util/tensor_shape_to_c_string.h>
+
 #include <executorch/runtime/platform/assert.h>
 
+#include <cinttypes>
+#include <cstdio> // For snprintf.
+#include <cstring>
+
 namespace executorch::runtime {
-/**
- * Shared implementation for tensor_util.h, may only contain code that
- * works whether or not ATen mode is active.
- */
-std::array<char, kTensorShapeStringSizeLimit> tensor_shape_to_c_string(
-    executorch::runtime::Span<const executorch::aten::SizesType> shape) {
+namespace {
+template <typename SizesType>
+std::array<char, kTensorShapeStringSizeLimit> tensor_shape_to_c_string_impl(
+    executorch::runtime::Span<SizesType> shape) {
   std::array<char, kTensorShapeStringSizeLimit> out;
   char* p = out.data();
   if ET_UNLIKELY (shape.size() > kTensorDimensionLimit) {
@@ -48,5 +50,16 @@ std::array<char, kTensorShapeStringSizeLimit> tensor_shape_to_c_string(
   *(p - 1) = '\0';
   return out;
 }
+} // namespace
+
+std::array<char, kTensorShapeStringSizeLimit> tensor_shape_to_c_string(
+    executorch::runtime::Span<const std::int32_t> shape) {
+  return tensor_shape_to_c_string_impl(shape);
+}
+
+std::array<char, kTensorShapeStringSizeLimit> tensor_shape_to_c_string(
+    executorch::runtime::Span<const std::int64_t> shape) {
+  return tensor_shape_to_c_string_impl(shape);
+}
 
 } // namespace executorch::runtime

kernels/portable/cpu/util/tensor_util.h renamed to runtime/core/exec_aten/util/tensor_shape_to_c_string.h

@@ -10,10 +10,10 @@
 
 #include <array>
 #include <cstddef>
+#include <cstdint>
 #include <limits>
 
-#include <executorch/runtime/core/exec_aten/exec_aten.h>
-#include <executorch/runtime/core/exec_aten/util/tensor_util.h>
+#include <executorch/runtime/core/exec_aten/util/tensor_dimension_limit.h>
 #include <executorch/runtime/core/span.h>
 
 namespace executorch::runtime {
@@ -34,18 +34,35 @@ constexpr size_t kTensorShapeStringSizeLimit = 1 + /* opening parenthesis */
 
 namespace internal {
 constexpr size_t kMaximumPrintableTensorShapeElement =
-    std::is_same_v<executorch::aten::SizesType, int32_t>
-    ? std::numeric_limits<int32_t>::max()
-    : std::numeric_limits<uint32_t>::max();
+    std::numeric_limits<int32_t>::max();
 } // namespace internal
 
 /**
  * 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<char, kTensorShapeStringSizeLimit> tensor_shape_to_c_string(
+    executorch::runtime::Span<const std::int32_t> 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<char, kTensorShapeStringSizeLimit> tensor_shape_to_c_string(
-    executorch::runtime::Span<const executorch::aten::SizesType> shape);
+    executorch::runtime::Span<const std::int64_t> shape);
 
 } // namespace executorch::runtime

runtime/core/exec_aten/util/tensor_util.h

@@ -20,6 +20,7 @@
 #include <executorch/runtime/core/exec_aten/exec_aten.h>
 #include <executorch/runtime/core/exec_aten/util/dim_order_util.h>
 #include <executorch/runtime/core/exec_aten/util/scalar_type_util.h>
+#include <executorch/runtime/core/exec_aten/util/tensor_dimension_limit.h>
 #include <executorch/runtime/core/span.h>
 #include <executorch/runtime/platform/assert.h>
 #include <executorch/runtime/platform/compiler.h>
@@ -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,

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)

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",
+        ],
+    )

kernels/portable/cpu/util/test/tensor_util_test.cpp renamed to runtime/core/exec_aten/util/test/tensor_shape_to_c_string_test.cpp

@@ -7,8 +7,8 @@
  */
 #include <gtest/gtest.h>
 
-#include <executorch/kernels/portable/cpu/util/tensor_util.h>
 #include <executorch/runtime/core/exec_aten/exec_aten.h>
+#include <executorch/runtime/core/exec_aten/util/tensor_shape_to_c_string.h>
 #include <executorch/runtime/platform/runtime.h>
 #include <array>
 
@@ -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<executorch::aten::SizesType, 3> sizes = {123, 456, 789};
   auto str = tensor_shape_to_c_string(
       Span<const executorch::aten::SizesType>(sizes.data(), sizes.size()));
@@ -29,7 +29,7 @@ TEST(TensorUtilTest, TensorShapeToCStringBasic) {
   EXPECT_STREQ(str.data(), "(1234567890)");
 }
 
-TEST(TensorUtilTest, TensorShapeToCStringNegativeItems) {
+TEST(TensorShapeToCStringTest, NegativeItems) {
   std::array<executorch::aten::SizesType, 4> sizes = {-1, -3, -2, 4};
   auto str = tensor_shape_to_c_string(
       Span<const executorch::aten::SizesType>(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<executorch::aten::SizesType, 3> sizes = {
       123, std::numeric_limits<executorch::aten::SizesType>::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<executorch::aten::SizesType, kTensorDimensionLimit> 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<executorch::aten::SizesType, kTensorDimensionLimit + 1> sizes;
   std::fill(sizes.begin(), sizes.end(), kMaximumPrintableTensorShapeElement);
 

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",
         ],
     )

runtime/core/portable_type/tensor_impl.cpp

@@ -13,6 +13,7 @@
 
 #include <executorch/runtime/core/exec_aten/util/dim_order_util.h>
 #include <executorch/runtime/core/exec_aten/util/scalar_type_util.h>
+#include <executorch/runtime/core/exec_aten/util/tensor_shape_to_c_string.h>
 #include <executorch/runtime/core/portable_type/qint_types.h>
 #include <executorch/runtime/core/portable_type/scalar_type.h>
 #include <executorch/runtime/platform/assert.h>
@@ -95,18 +96,12 @@ Error TensorImpl::internal_resize_contiguous(ArrayRef<SizesType> new_sizes) {
     case TensorShapeDynamism::STATIC:
       if (!std::equal(sizes_, sizes_ + dim_, new_sizes.begin())) {
 #ifdef ET_LOG_ENABLED
-        std::array<char, 16> 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<const SizesType>(
+                sizes().data(), sizes().size()));
+        auto new_sizes_str = executorch::runtime::tensor_shape_to_c_string(
+            executorch::runtime::Span<const SizesType>(
+                new_sizes.data(), new_sizes.size()));
 #endif
 
         ET_CHECK_OR_RETURN_ERROR(