From 3c3f17fbe65b2f6b305038bfa7823d755bb8c19a Mon Sep 17 00:00:00 2001 From: Songhao Jia Date: Wed, 2 Apr 2025 01:51:56 -0700 Subject: [PATCH] introduce DelegateDebugIntId Summary: EventTracer misused `DebugHandle` type (uint32_t) for debug id of both op-level and delegation level. In fact for delegate_debug_id we should use int32_t instead. This diff creates a new type called `DelegateDebugIntId` specific for delegate_debug_id, and a new flag kUnsetDelegateDebugIntId for unset delegate debug id. Updates corresponding tests as well. Differential Revision: D72297495 --- devtools/etdump/etdump_flatcc.cpp | 26 ++++++++------- devtools/etdump/etdump_flatcc.h | 17 +++++----- devtools/etdump/tests/etdump_test.cpp | 42 +++++++++---------------- runtime/core/event_tracer.h | 29 ++++++++++++----- runtime/core/test/event_tracer_test.cpp | 20 ++++++------ 5 files changed, 69 insertions(+), 65 deletions(-) diff --git a/devtools/etdump/etdump_flatcc.cpp b/devtools/etdump/etdump_flatcc.cpp index 461901834d2..5f419ba0503 100644 --- a/devtools/etdump/etdump_flatcc.cpp +++ b/devtools/etdump/etdump_flatcc.cpp @@ -27,8 +27,10 @@ using ::executorch::runtime::ArrayRef; using ::executorch::runtime::ChainID; using ::executorch::runtime::DebugHandle; using ::executorch::runtime::DelegateDebugIdType; +using ::executorch::runtime::DelegateDebugIntId; using ::executorch::runtime::EValue; using ::executorch::runtime::EventTracerEntry; +using ::executorch::runtime::kUnsetDelegateDebugIntId; using ::executorch::runtime::LoggedEValueType; using ::executorch::runtime::Result; using ::executorch::runtime::Span; @@ -223,9 +225,9 @@ EventTracerEntry ETDumpGen::start_profiling( // EventTracerEntry struct is updated. EventTracerEntry ETDumpGen::start_profiling_delegate( const char* name, - DebugHandle delegate_debug_index) { + DelegateDebugIntId delegate_debug_index) { ET_CHECK_MSG( - (name == nullptr) ^ (delegate_debug_index == -1), + (name == nullptr) ^ (delegate_debug_index == kUnsetDelegateDebugIntId), "Only name or delegate_debug_index can be valid. Check DelegateMappingBuilder documentation for more details."); check_ready_to_add_events(); EventTracerEntry prof_entry; @@ -234,7 +236,7 @@ EventTracerEntry ETDumpGen::start_profiling_delegate( prof_entry.delegate_event_id_type = delegate_event_id_type; prof_entry.chain_id = chain_id_; prof_entry.debug_handle = debug_handle_; - prof_entry.event_id = delegate_debug_index == static_cast(-1) + prof_entry.event_id = delegate_debug_index == kUnsetDelegateDebugIntId ? create_string_entry(name) : delegate_debug_index; prof_entry.start_time = et_pal_current_ticks(); @@ -276,13 +278,13 @@ void ETDumpGen::end_profiling_delegate( void ETDumpGen::log_profiling_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, et_timestamp_t start_time, et_timestamp_t end_time, const void* metadata, size_t metadata_len) { ET_CHECK_MSG( - (name == nullptr) ^ (delegate_debug_index == -1), + (name == nullptr) ^ (delegate_debug_index == kUnsetDelegateDebugIntId), "Only name or delegate_debug_index can be valid. Check DelegateMappingBuilder documentation for more details."); check_ready_to_add_events(); int64_t string_id = name != nullptr ? create_string_entry(name) : -1; @@ -308,7 +310,7 @@ void ETDumpGen::log_profiling_delegate( Result ETDumpGen::log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const Tensor& output) { Result result = log_intermediate_output_delegate_helper( name, delegate_debug_index, output); @@ -317,7 +319,7 @@ Result ETDumpGen::log_intermediate_output_delegate( Result ETDumpGen::log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const ArrayRef output) { log_intermediate_output_delegate_helper(name, delegate_debug_index, output); Result result = log_intermediate_output_delegate_helper( @@ -327,7 +329,7 @@ Result ETDumpGen::log_intermediate_output_delegate( Result ETDumpGen::log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const int& output) { log_intermediate_output_delegate_helper(name, delegate_debug_index, output); Result result = log_intermediate_output_delegate_helper( @@ -337,7 +339,7 @@ Result ETDumpGen::log_intermediate_output_delegate( Result ETDumpGen::log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const bool& output) { log_intermediate_output_delegate_helper(name, delegate_debug_index, output); Result result = log_intermediate_output_delegate_helper( @@ -347,7 +349,7 @@ Result ETDumpGen::log_intermediate_output_delegate( Result ETDumpGen::log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const double& output) { log_intermediate_output_delegate_helper(name, delegate_debug_index, output); Result result = log_intermediate_output_delegate_helper( @@ -358,10 +360,10 @@ Result ETDumpGen::log_intermediate_output_delegate( template Result ETDumpGen::log_intermediate_output_delegate_helper( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const T& output) { ET_CHECK_OR_RETURN_ERROR( - (name == nullptr) ^ (delegate_debug_index == -1), + (name == nullptr) ^ (delegate_debug_index == kUnsetDelegateDebugIntId), InvalidArgument, "Only name or delegate_debug_index can be valid. Check DelegateMappingBuilder documentation for more details."); diff --git a/devtools/etdump/etdump_flatcc.h b/devtools/etdump/etdump_flatcc.h index 4aff62a8767..b3588edbc0b 100644 --- a/devtools/etdump/etdump_flatcc.h +++ b/devtools/etdump/etdump_flatcc.h @@ -25,6 +25,7 @@ struct flatcc_builder; namespace executorch { namespace etdump { +using ::executorch::runtime::DelegateDebugIntId; using ::executorch::runtime::Result; namespace internal { @@ -84,14 +85,14 @@ class ETDumpGen : public ::executorch::runtime::EventTracer { ::executorch::runtime::EventTracerEntry prof_entry) override; virtual ::executorch::runtime::EventTracerEntry start_profiling_delegate( const char* name, - ::executorch::runtime::DebugHandle delegate_debug_index) override; + DelegateDebugIntId delegate_debug_index) override; virtual void end_profiling_delegate( ::executorch::runtime::EventTracerEntry prof_entry, const void* metadata, size_t metadata_len) override; virtual void log_profiling_delegate( const char* name, - ::executorch::runtime::DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, et_timestamp_t start_time, et_timestamp_t end_time, const void* metadata, @@ -111,7 +112,7 @@ class ETDumpGen : public ::executorch::runtime::EventTracer { */ virtual Result log_intermediate_output_delegate( const char* name, - ::executorch::runtime::DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const executorch::aten::Tensor& output) override; /** @@ -119,7 +120,7 @@ class ETDumpGen : public ::executorch::runtime::EventTracer { */ virtual Result log_intermediate_output_delegate( const char* name, - ::executorch::runtime::DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const ::executorch::runtime::ArrayRef output) override; @@ -128,7 +129,7 @@ class ETDumpGen : public ::executorch::runtime::EventTracer { */ virtual Result log_intermediate_output_delegate( const char* name, - ::executorch::runtime::DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const int& output) override; /** @@ -136,7 +137,7 @@ class ETDumpGen : public ::executorch::runtime::EventTracer { */ virtual Result log_intermediate_output_delegate( const char* name, - ::executorch::runtime::DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const bool& output) override; /** @@ -144,7 +145,7 @@ class ETDumpGen : public ::executorch::runtime::EventTracer { */ virtual Result log_intermediate_output_delegate( const char* name, - ::executorch::runtime::DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const double& output) override; void set_debug_buffer(::executorch::runtime::Span buffer); void set_data_sink(DataSinkBase* data_sink); @@ -173,7 +174,7 @@ class ETDumpGen : public ::executorch::runtime::EventTracer { template Result log_intermediate_output_delegate_helper( const char* name, - ::executorch::runtime::DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const T& output); long write_tensor_or_raise_error(executorch::aten::Tensor tensor); diff --git a/devtools/etdump/tests/etdump_test.cpp b/devtools/etdump/tests/etdump_test.cpp index 658672c6646..c64bab0448c 100644 --- a/devtools/etdump/tests/etdump_test.cpp +++ b/devtools/etdump/tests/etdump_test.cpp @@ -36,6 +36,7 @@ using ::executorch::runtime::DelegateDebugIdType; using ::executorch::runtime::Error; using ::executorch::runtime::EValue; using ::executorch::runtime::EventTracerEntry; +using ::executorch::runtime::kUnsetDelegateDebugIntId; using ::executorch::runtime::LoggedEValueType; using ::executorch::runtime::Span; using ::executorch::runtime::Tag; @@ -70,9 +71,7 @@ class ProfilerETDumpTest : public ::testing::Test { TensorFactory& tf) { ET_EXPECT_DEATH( gen->log_intermediate_output_delegate( - "test_event_tensor", - static_cast(-1), - tf.ones({3, 2})), + "test_event_tensor", kUnsetDelegateDebugIntId, tf.ones({3, 2})), "Must set data sink before writing tensor-like data"); } @@ -582,7 +581,7 @@ TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { Result log_tensor_list_result = etdump_gen[i]->log_intermediate_output_delegate( nullptr, - static_cast(-1), + kUnsetDelegateDebugIntId, ArrayRef(tensors.data(), tensors.size())); Result log_int_result = @@ -599,7 +598,7 @@ TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { Result log_bool_result = etdump_gen[i]->log_intermediate_output_delegate( - nullptr, static_cast(-1), 29.82); + nullptr, kUnsetDelegateDebugIntId, 29.82); ASSERT_EQ(log_tensor_result.error(), Error::InvalidArgument); ASSERT_EQ(log_tensor_list_result.error(), Error::InvalidArgument); @@ -611,33 +610,25 @@ TEST_F(ProfilerETDumpTest, LogDelegateIntermediateOutput) { // Log a tensor etdump_gen[i]->log_intermediate_output_delegate( - "test_event_tensor", - static_cast(-1), - tf.ones({3, 2})); + "test_event_tensor", kUnsetDelegateDebugIntId, tf.ones({3, 2})); // Log a tensor list etdump_gen[i]->log_intermediate_output_delegate( "test_event_tensorlist", - static_cast(-1), + kUnsetDelegateDebugIntId, ArrayRef(tensors.data(), tensors.size())); // Log an int etdump_gen[i]->log_intermediate_output_delegate( - "test_event_tensorlist", - static_cast(-1), - 10); + "test_event_tensorlist", kUnsetDelegateDebugIntId, 10); // Log a double etdump_gen[i]->log_intermediate_output_delegate( - "test_event_tensorlist", - static_cast(-1), - 20.75); + "test_event_tensorlist", kUnsetDelegateDebugIntId, 20.75); // Log a bool etdump_gen[i]->log_intermediate_output_delegate( - "test_event_tensorlist", - static_cast(-1), - true); + "test_event_tensorlist", kUnsetDelegateDebugIntId, true); ETDumpResult result = etdump_gen[i]->get_etdump_data(); ASSERT_TRUE(result.buf != nullptr); @@ -762,23 +753,18 @@ TEST_F(ProfilerETDumpTest, LogDelegateEvents) { etdump_gen[i]->log_profiling_delegate( nullptr, 278, 1, 2, metadata, strlen(metadata) + 1); EventTracerEntry entry = etdump_gen[i]->start_profiling_delegate( - "test_event", static_cast(-1)); + "test_event", kUnsetDelegateDebugIntId); EXPECT_NE(entry.delegate_event_id_type, DelegateDebugIdType::kNone); // Event 2 etdump_gen[i]->end_profiling_delegate( entry, metadata, strlen(metadata) + 1); // Event 3 etdump_gen[i]->log_profiling_delegate( - "test_event", - static_cast(-1), - 1, - 2, - nullptr, - 0); + "test_event", kUnsetDelegateDebugIntId, 1, 2, nullptr, 0); // Event 4 etdump_gen[i]->log_profiling_delegate( "test_event", - static_cast(-1), + kUnsetDelegateDebugIntId, 1, 2, metadata, @@ -856,11 +842,11 @@ TEST_F(ProfilerETDumpTest, LogDelegateEvents) { std::string(delegate_debug_id_name, strlen(delegate_debug_id_name)), "test_event"); // Event 2 used a string delegate debug identifier, so delegate_debug_id_int - // should be -1. + // should be kUnsetDelegateDebugIntId. EXPECT_EQ( etdump_ProfileEvent_delegate_debug_id_int( etdump_Event_profile_event(event)), - -1); + kUnsetDelegateDebugIntId); if (!etdump_gen[i]->is_static_etdump()) { free(result.buf); } diff --git a/runtime/core/event_tracer.h b/runtime/core/event_tracer.h index b55e90f0b7a..eecd1381e79 100644 --- a/runtime/core/event_tracer.h +++ b/runtime/core/event_tracer.h @@ -26,10 +26,14 @@ typedef int32_t ChainID; /// Represents the debug handle that is generally associated with each /// op executed in the runtime. typedef uint32_t DebugHandle; +// Represents the delegate debug id that is generally associated with each +// delegate event. +typedef int32_t DelegateDebugIntId; /// Default id's for chain id and debug handle. constexpr ChainID kUnsetChainId = -1; constexpr DebugHandle kUnsetDebugHandle = 0; +constexpr DelegateDebugIntId kUnsetDelegateDebugIntId = -1; // Default bundled input index to indicate that it hasn't been set yet. constexpr int kUnsetBundledInputIndex = -1; @@ -96,7 +100,9 @@ class EventTracerFilterBase { * - False if the event does not match or is unknown. * - An error code if an error occurs during filtering. */ - virtual Result filter(char* name, DebugHandle delegate_debug_index); + virtual Result filter( + char* name, + DelegateDebugIntId delegate_debug_index); /** * Virtual destructor for the EventTracerFilterBase class. @@ -122,6 +128,11 @@ enum class EventTracerProfilingLevel { * started. This is used to uniquely identify that profiling event and will be * required to be passed into the end_profiling call to signal that the event * identified by this struct has completed. + * + * TODO(gasoonjia): Now this struct is mix-used for both delegate and + *non-delegate events. In the future we should separate them into two different + *structs: EventTracerEntry for non-delegate events holding DebugHandle, and + *DelegateEventTracerEntry for delegate events holding DelegateDebugIntId. **/ struct EventTracerEntry { /// An event id to uniquely identify this event that was generated during a @@ -208,7 +219,7 @@ class EventTracer { */ virtual EventTracerEntry start_profiling_delegate( const char* name, - DebugHandle delegate_debug_index) = 0; + DelegateDebugIntId delegate_debug_index) = 0; /** * Signal the end of the delegate profiling event contained in @@ -256,7 +267,7 @@ class EventTracer { */ virtual void log_profiling_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, et_timestamp_t start_time, et_timestamp_t end_time, const void* metadata = nullptr, @@ -328,7 +339,7 @@ class EventTracer { */ virtual Result log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const executorch::aten::Tensor& output) = 0; /** @@ -353,7 +364,7 @@ class EventTracer { */ virtual Result log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const ArrayRef output) = 0; /** @@ -377,7 +388,7 @@ class EventTracer { */ virtual Result log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const int& output) = 0; /** @@ -401,7 +412,7 @@ class EventTracer { */ virtual Result log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const bool& output) = 0; /** @@ -425,7 +436,7 @@ class EventTracer { */ virtual Result log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const double& output) = 0; /** @@ -565,12 +576,14 @@ using ::executorch::runtime::AllocatorID; using ::executorch::runtime::ChainID; using ::executorch::runtime::DebugHandle; using ::executorch::runtime::DelegateDebugIdType; +using ::executorch::runtime::DelegateDebugIntId; using ::executorch::runtime::EventTracer; using ::executorch::runtime::EventTracerDebugLogLevel; using ::executorch::runtime::EventTracerEntry; using ::executorch::runtime::kUnsetBundledInputIndex; using ::executorch::runtime::kUnsetChainId; using ::executorch::runtime::kUnsetDebugHandle; +using ::executorch::runtime::kUnsetDelegateDebugIntId; using ::executorch::runtime::LoggedEValueType; } // namespace executor } // namespace torch diff --git a/runtime/core/test/event_tracer_test.cpp b/runtime/core/test/event_tracer_test.cpp index 310483fab49..224e87cc2b1 100644 --- a/runtime/core/test/event_tracer_test.cpp +++ b/runtime/core/test/event_tracer_test.cpp @@ -23,12 +23,14 @@ using executorch::runtime::AllocatorID; using executorch::runtime::ArrayRef; using executorch::runtime::ChainID; using executorch::runtime::DebugHandle; +using executorch::runtime::DelegateDebugIntId; using executorch::runtime::EValue; using executorch::runtime::EventTracer; using executorch::runtime::EventTracerDebugLogLevel; using executorch::runtime::EventTracerEntry; using executorch::runtime::kUnsetChainId; using executorch::runtime::kUnsetDebugHandle; +using executorch::runtime::kUnsetDelegateDebugIntId; using executorch::runtime::LoggedEValueType; using executorch::runtime::Result; @@ -73,7 +75,7 @@ class DummyEventTracer : public EventTracer { EventTracerEntry start_profiling_delegate( const char* name, - DebugHandle delegate_debug_id) override { + DelegateDebugIntId delegate_debug_id) override { (void)name; (void)delegate_debug_id; return EventTracerEntry(); @@ -90,7 +92,7 @@ class DummyEventTracer : public EventTracer { void log_profiling_delegate( const char* name, - DebugHandle delegate_debug_id, + DelegateDebugIntId delegate_debug_id, et_timestamp_t start_time, et_timestamp_t end_time, const void* metadata, @@ -105,7 +107,7 @@ class DummyEventTracer : public EventTracer { virtual Result log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const Tensor& output) override { (void)name; (void)delegate_debug_index; @@ -115,7 +117,7 @@ class DummyEventTracer : public EventTracer { virtual Result log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const ArrayRef output) override { (void)name; (void)delegate_debug_index; @@ -125,7 +127,7 @@ class DummyEventTracer : public EventTracer { virtual Result log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const int& output) override { (void)name; (void)delegate_debug_index; @@ -135,7 +137,7 @@ class DummyEventTracer : public EventTracer { virtual Result log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const bool& output) override { (void)name; (void)delegate_debug_index; @@ -145,7 +147,7 @@ class DummyEventTracer : public EventTracer { virtual Result log_intermediate_output_delegate( const char* name, - DebugHandle delegate_debug_index, + DelegateDebugIntId delegate_debug_index, const double& output) override { (void)name; (void)delegate_debug_index; @@ -226,14 +228,14 @@ TEST(TestEventTracer, SimpleEventTracerTest) { */ void RunSimpleTracerTestDelegate(EventTracer* event_tracer) { EventTracerEntry event_tracer_entry = event_tracer_start_profiling_delegate( - event_tracer, "test_event", kUnsetDebugHandle); + event_tracer, "test_event", kUnsetDelegateDebugIntId); event_tracer_end_profiling_delegate( event_tracer, event_tracer_entry, nullptr); event_tracer_start_profiling_delegate(event_tracer, nullptr, 1); event_tracer_end_profiling_delegate( event_tracer, event_tracer_entry, "test_metadata"); event_tracer_log_profiling_delegate( - event_tracer, "test_event", kUnsetDebugHandle, 0, 1, nullptr); + event_tracer, "test_event", kUnsetDelegateDebugIntId, 0, 1, nullptr); event_tracer_log_profiling_delegate(event_tracer, nullptr, 1, 0, 1, nullptr); }