From 02da069bb403f5afef591ac8b28056fd239b9ab4 Mon Sep 17 00:00:00 2001 From: Gregory Comer Date: Wed, 22 Jan 2025 09:39:17 -0800 Subject: [PATCH] Log EValue tag names instead of numeric values Differential Revision: D67888756 Pull Request resolved: https://github.com/pytorch/executorch/pull/7538 --- runtime/core/defines.h | 20 ++++++++ runtime/core/tag.cpp | 52 +++++++++++++++++++++ runtime/core/tag.h | 23 ++++++++++ runtime/core/targets.bzl | 15 ++++++ runtime/core/test/tag_test.cpp | 80 ++++++++++++++++++++++++++++++++ runtime/core/test/targets.bzl | 10 ++++ runtime/executor/method.cpp | 84 +++++++++++++++++++++++----------- 7 files changed, 258 insertions(+), 26 deletions(-) create mode 100644 runtime/core/defines.h create mode 100644 runtime/core/tag.cpp create mode 100644 runtime/core/test/tag_test.cpp diff --git a/runtime/core/defines.h b/runtime/core/defines.h new file mode 100644 index 0000000000..ee47126826 --- /dev/null +++ b/runtime/core/defines.h @@ -0,0 +1,20 @@ +/* + * 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. + */ + +/** + * @file + * Contains preprocessor definitions used by ExecuTorch core. + */ + +#pragma once + +// Enable ET_ENABLE_ENUM_STRINGS by default. This option gates inclusion of +// enum string names and can be disabled by explicitly setting it to 0. +#ifndef ET_ENABLE_ENUM_STRINGS +#define ET_ENABLE_ENUM_STRINGS 1 +#endif diff --git a/runtime/core/tag.cpp b/runtime/core/tag.cpp new file mode 100644 index 0000000000..fad5aae4f0 --- /dev/null +++ b/runtime/core/tag.cpp @@ -0,0 +1,52 @@ +/* + * 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. + */ + +#include + +#include + +namespace executorch { +namespace runtime { + +/** + * Convert a tag value to a string representation. If ET_ENABLE_ENUM_STRINGS is + * set (it is on by default), this will return a string name (for example, + * "Tensor"). Otherwise, it will return a string representation of the index + * value ("1"). + * + * If the user buffer is not large enough to hold the string representation, the + * string will be truncated. + * + * The return value is the number of characters written, or in the case of + * truncation, the number of characters that would be written if the buffer was + * large enough. + */ +size_t tag_to_string(Tag tag, char* buffer, size_t buffer_size) { +#if ET_ENABLE_ENUM_STRINGS + const char* name_str; +#define DEFINE_CASE(name) \ + case Tag::name: \ + name_str = #name; \ + break; + + switch (tag) { + EXECUTORCH_FORALL_TAGS(DEFINE_CASE) + default: + name_str = "Unknown"; + break; + } + + return snprintf(buffer, buffer_size, "%s", name_str); +#undef DEFINE_CASE +#else + return snprintf(buffer, buffer_size, "%d", static_cast(tag)); +#endif // ET_ENABLE_ENUM_TO_STRING +} + +} // namespace runtime +} // namespace executorch diff --git a/runtime/core/tag.h b/runtime/core/tag.h index 7dda80dd92..db70c9879a 100644 --- a/runtime/core/tag.h +++ b/runtime/core/tag.h @@ -8,6 +8,8 @@ #pragma once +#include +#include #include namespace executorch { @@ -36,6 +38,27 @@ enum class Tag : uint32_t { #undef DEFINE_TAG }; +/** + * Convert a tag value to a string representation. If ET_ENABLE_ENUM_STRINGS is + * set (it is on by default), this will return a string name (for example, + * "Tensor"). Otherwise, it will return a string representation of the index + * value ("1"). + * + * If the user buffer is not large enough to hold the string representation, the + * string will be truncated. + * + * The return value is the number of characters written, or in the case of + * truncation, the number of characters that would be written if the buffer was + * large enough. + */ +size_t tag_to_string(Tag tag, char* buffer, size_t buffer_size); + +/* The size of the buffer needed to hold the longest tag string, including the + * null terminator. This value is expected to be updated manually, but it + * checked in test_tag.cpp. + */ +constexpr size_t kTagNameBufferSize = 19; + } // namespace runtime } // namespace executorch diff --git a/runtime/core/targets.bzl b/runtime/core/targets.bzl index 7e0aeb5d28..352c962eb2 100644 --- a/runtime/core/targets.bzl +++ b/runtime/core/targets.bzl @@ -18,6 +18,14 @@ def get_sdk_flags(): sdk_flags += ["-DEXECUTORCH_BUILD_DEVTOOLS"] return sdk_flags +def enable_enum_strings(): + return native.read_config("executorch", "enable_enum_strings", "true") == "true" + +def get_core_flags(): + core_flags = [] + core_flags += ["-DET_ENABLE_ENUM_STRINGS=" + ("1" if enable_enum_strings() else "0")] + return core_flags + def define_common_targets(): """Defines targets that should be shared between fbcode and xplat. @@ -30,6 +38,7 @@ def define_common_targets(): exported_headers = [ "array_ref.h", # TODO(T157717874): Migrate all users to span and then move this to portable_type "data_loader.h", + "defines.h", "error.h", "freeable_buffer.h", "result.h", @@ -39,6 +48,7 @@ def define_common_targets(): "//executorch/...", "@EXECUTORCH_CLIENTS", ], + exported_preprocessor_flags = get_core_flags(), exported_deps = [ "//executorch/runtime/platform:platform", ], @@ -109,9 +119,14 @@ def define_common_targets(): runtime.cxx_library( name = "tag", + srcs = ["tag.cpp"], exported_headers = [ "tag.h", ], + exported_deps = [ + ":core", + "//executorch/runtime/platform:compiler", + ], visibility = [ "//executorch/...", ], diff --git a/runtime/core/test/tag_test.cpp b/runtime/core/test/tag_test.cpp new file mode 100644 index 0000000000..4b41aded42 --- /dev/null +++ b/runtime/core/test/tag_test.cpp @@ -0,0 +1,80 @@ +/* + * 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. + */ + +#include + +#include +#include + +using namespace ::testing; +using executorch::runtime::kTagNameBufferSize; +using executorch::runtime::Tag; +using executorch::runtime::tag_to_string; + +// The behavior of tag_to_string depends on the value of ET_ENABLE_ENUM_STRINGS. +// If it is not set, tag_to_string will return a string representation of the +// enum index value. As this behavior is compile-time gated, tests must also +// be compile-time gated. +#if ET_ENABLE_ENUM_STRINGS +TEST(TagToString, TagValues) { + std::array name; + + tag_to_string(Tag::Tensor, name.data(), name.size()); + EXPECT_STREQ("Tensor", name.data()); + + tag_to_string(Tag::Int, name.data(), name.size()); + EXPECT_STREQ("Int", name.data()); + + tag_to_string(Tag::Double, name.data(), name.size()); + EXPECT_STREQ("Double", name.data()); + + tag_to_string(Tag::Bool, name.data(), name.size()); + EXPECT_STREQ("Bool", name.data()); +} + +TEST(TagToString, TagNameBufferSize) { + // Validate that kTagNameBufferSize is large enough to hold the all tag + // strings without truncation. + std::array name; + + // Note that the return value of tag_to_string does not include the null + // terminator. + size_t longest = 0; + +#define TEST_CASE(tag) \ + auto tag##_len = tag_to_string(Tag::tag, name.data(), name.size()); \ + EXPECT_LT(tag##_len, kTagNameBufferSize) \ + << "kTagNameBufferSize is too small to hold " #tag; \ + longest = std::max(longest, tag##_len); + + EXECUTORCH_FORALL_TAGS(TEST_CASE) +#undef TEST_CASE + + EXPECT_EQ(longest + 1, kTagNameBufferSize) + << "kTagNameBufferSize has incorrect value, expected " << longest + 1; +} + +TEST(TagToString, FitsExact) { + std::array name; + + auto ret = tag_to_string(Tag::Int, name.data(), name.size()); + + EXPECT_EQ(3, ret); + EXPECT_STREQ("Int", name.data()); +} + +TEST(TagToString, Truncate) { + std::array name; + std::fill(name.begin(), name.end(), '-'); + + auto ret = tag_to_string(Tag::Double, name.data(), name.size()); + EXPECT_EQ(6, ret); + EXPECT_TRUE(name[name.size() - 1] == 0); + EXPECT_STREQ("Doubl", name.data()); +} +#endif // ET_ENABLE_ENUM_STRINGS diff --git a/runtime/core/test/targets.bzl b/runtime/core/test/targets.bzl index 2857de308b..d221d49e6e 100644 --- a/runtime/core/test/targets.bzl +++ b/runtime/core/test/targets.bzl @@ -73,6 +73,16 @@ def define_common_targets(): ], ) + runtime.cxx_test( + name = "tag_test", + srcs = [ + "tag_test.cpp", + ], + deps = [ + "//executorch/runtime/core:tag", + ], + ) + runtime.cxx_test( name = "tensor_shape_dynamism_test_aten", srcs = ["tensor_shape_dynamism_test_aten.cpp"], diff --git a/runtime/executor/method.cpp b/runtime/executor/method.cpp index 41b2f1225b..8587626405 100644 --- a/runtime/executor/method.cpp +++ b/runtime/executor/method.cpp @@ -8,6 +8,7 @@ #include +#include #include // @donotremove #include #include @@ -823,26 +824,43 @@ Method::set_input(const EValue& input_evalue, size_t input_idx) { ET_CHECK_OR_RETURN_ERROR( input_idx < inputs_size(), InvalidArgument, - "Given input index must be less than the number of inputs in method, but got %zu and %zu", + "Input index (%zu) must be less than the number of inputs in method (%zu).", input_idx, inputs_size()); const auto& e = get_value(get_input_index(input_idx)); - ET_CHECK_OR_RETURN_ERROR( - e.isTensor() || e.isScalar(), - InvalidArgument, - "The %zu-th input in method is expected Tensor or prim, but received %" PRIu32, - input_idx, - static_cast(e.tag)); - ET_CHECK_OR_RETURN_ERROR( - e.tag == input_evalue.tag, - InvalidArgument, - "The %zu-th input of method should have the same type as the input_evalue, but get tag %" PRIu32 - " and tag %" PRIu32, - input_idx, - static_cast(e.tag), - static_cast(input_evalue.tag)); + if (!e.isTensor() && !e.isScalar()) { +#if ET_LOG_ENABLED + std::array tag_name; + tag_to_string(e.tag, tag_name.data(), tag_name.size()); + ET_LOG( + Error, + "Input %zu was expected to be a Tensor or primitive but was %s.", + input_idx, + tag_name.data()); +#endif + + return Error::InvalidArgument; + } + + if (e.tag != input_evalue.tag) { +#if ET_LOG_ENABLED + std::array e_tag_name; + std::array input_tag_name; + tag_to_string(e.tag, e_tag_name.data(), e_tag_name.size()); + tag_to_string( + input_evalue.tag, input_tag_name.data(), input_tag_name.size()); + ET_LOG( + Error, + "Input %zu was expected to have type %s but was %s.", + input_idx, + e_tag_name.data(), + input_tag_name.data()); +#endif + + return Error::InvalidArgument; + } if (e.isTensor()) { const auto& t_dst = e.toTensor(); @@ -932,7 +950,12 @@ Method::set_input(const EValue& input_evalue, size_t input_idx) { e.toString().data(), input_evalue.toString().data()); } else { - ET_LOG(Error, "Unsupported input type: %d", (int32_t)e.tag); +#if ET_LOG_ENABLED + std::array tag_name; + tag_to_string(e.tag, tag_name.data(), tag_name.size()); + ET_LOG(Error, "Unsupported input type: %s", tag_name.data()); +#endif + return Error::InvalidArgument; } return Error::Ok; @@ -984,11 +1007,15 @@ Method::set_output_data_ptr(void* buffer, size_t size, size_t output_idx) { outputs_size()); auto& output = mutable_value(get_output_index(output_idx)); - ET_CHECK_OR_RETURN_ERROR( - output.isTensor(), - InvalidArgument, - "output type: %zu is not tensor", - (size_t)output.tag); + if (!output.isTensor()) { +#if ET_LOG_ENABLED + std::array tag_name; + tag_to_string(output.tag, tag_name.data(), tag_name.size()); + ET_LOG(Error, "Output type: %s is not a tensor.", tag_name.data()); +#endif + + return Error::InvalidArgument; + } auto tensor_meta = this->method_meta().output_tensor_meta(output_idx); if (tensor_meta->is_memory_planned()) { @@ -1001,11 +1028,16 @@ Method::set_output_data_ptr(void* buffer, size_t size, size_t output_idx) { } auto& t = output.toTensor(); - ET_CHECK_OR_RETURN_ERROR( - output.isTensor(), - InvalidArgument, - "output type: %zu is not tensor", - (size_t)output.tag); + if (!output.isTensor()) { +#if ET_LOG_ENABLED + std::array tag_name; + tag_to_string(output.tag, tag_name.data(), tag_name.size()); + ET_LOG(Error, "output type: %s is not a tensor.", tag_name.data()); +#endif + + return Error::InvalidArgument; + } + ET_CHECK_OR_RETURN_ERROR( t.nbytes() <= size, InvalidArgument,