[runtime][hip] Add tracing in graph execution mode #16894

sogartar · 2024-03-25T23:54:02Z

There are some outstanding concurrency issues where some events don't get recorded during collection.
If we collect only when the device gets destroyed with a 1s delay, the concurrency issues seem to not manifest.

sogartar · 2024-03-26T00:00:17Z

Here is the change that collects events only on device destruction https://github.com/sogartar/SHARK-Runtime/tree/hip-graph-tracing-collect-only-on-device-destruction
There I don't see concurrency issues with respect to tracing.
I think we need to resolve these concurrency issues in separate PR.
Although, I am fairly positive they don't come form the tracing logic introduced here, I urge the reviewers to be alert since synchronization logic is easy to get wrong and hard to debug.

nithinsubbiah · 2024-03-26T11:57:47Z

I saw internally that you're testing the latest branch to make sure the concurrency problem isn't with the HIP API. Please update your findings and we can go forward with this patch.

sogartar · 2024-03-26T14:21:58Z

@nithinsubbiah here is an issue #16903.

nithinsubbiah · 2024-03-26T16:24:04Z

Thanks Boian, I have seen some failures on the cts earlier. I'm looking into them.

antiagainst · 2024-04-04T16:25:07Z

runtime/src/iree/hal/drivers/hip/dynamic_symbol_tables.h

@@ -54,6 +54,9 @@ IREE_HAL_HIP_REQUIRED_PFN_DECL(hipGraphExecDestroy, hipGraphExec_t)
 IREE_HAL_HIP_REQUIRED_PFN_DECL(hipGraphInstantiate, hipGraphExec_t *,
                               hipGraph_t, hipGraphNode_t *, char *, size_t)
 IREE_HAL_HIP_REQUIRED_PFN_DECL(hipGraphLaunch, hipGraphExec_t, hipStream_t)
+IREE_HAL_HIP_REQUIRED_PFN_DECL(hipGraphAddEventRecordNode, hipGraphNode_t *,


Sort these APIs alphabetically

antiagainst · 2024-04-04T17:01:06Z

runtime/src/iree/hal/drivers/hip/tracing.h

@@ -77,12 +76,22 @@ void iree_hal_hip_tracing_zone_begin_external_impl(
    const char* file_name, size_t file_name_length, uint32_t line,
    const char* function_name, size_t function_name_length, const char* name,
    size_t name_length);
+void iree_hal_hip_graph_tracing_zone_begin_external_impl(


Can up update the existing tracing APIs to include "stream" inside to better differentiate?

antiagainst · 2024-04-04T23:24:55Z

runtime/src/iree/hal/drivers/hip/tracing.c

@@ -249,7 +275,10 @@ static uint16_t iree_hal_hip_tracing_context_insert_query(
 void iree_hal_hip_tracing_zone_begin_impl(
    iree_hal_hip_tracing_context_t* context, hipStream_t stream,
    const iree_tracing_location_t* src_loc) {
-  if (!context) return;
+  if (!context) {


Use IREE_ASSERT_ARGUMENT

antiagainst · 2024-04-04T23:25:14Z

runtime/src/iree/hal/drivers/hip/tracing.c

@@ -260,14 +289,31 @@ void iree_hal_hip_tracing_zone_begin_external_impl(
    const char* file_name, size_t file_name_length, uint32_t line,
    const char* function_name, size_t function_name_length, const char* name,
    size_t name_length) {
-  if (!context) return;
+  if (!context) {


antiagainst · 2024-04-04T23:27:21Z

runtime/src/iree/hal/drivers/hip/tracing.h


 void iree_hal_hip_tracing_zone_end_impl(iree_hal_hip_tracing_context_t* context,
                                        hipStream_t stream);
+void iree_hal_hip_graph_tracing_zone_end_impl(


antiagainst · 2024-04-04T23:29:19Z

runtime/src/iree/hal/drivers/hip/graph_command_buffer.c

@@ -11,11 +11,13 @@
 #include <stdint.h>

 #include "iree/base/api.h"
+#include "iree/base/attributes.h"


This is included as part of base/api.h so no need to pull in specifically

antiagainst · 2024-04-04T23:31:24Z

runtime/src/iree/hal/drivers/hip/pending_queue_actions.c

@@ -593,6 +593,10 @@ static iree_status_t iree_hal_hip_pending_queue_actions_issue_execution(
  }

  // Then launch all command buffers to the dispatch stream.
+  IREE_TRACE_ZONE_BEGIN(zone_launch_command_buffers);


No need to prefix the zone name with zone_. Maybe just call it "dispatch_command_buffers".

antiagainst · 2024-04-04T23:44:44Z

runtime/src/iree/hal/drivers/hip/graph_command_buffer.c

+      iree_hal_hip_graph_command_buffer_cast(base_command_buffer);
+  IREE_TRACE_ZONE_BEGIN(z0);
+
+  IREE_RETURN_AND_END_ZONE_IF_ERROR(


Just do a normal iree_status_t status = and return status later.

antiagainst · 2024-04-04T23:47:15Z

runtime/src/iree/hal/drivers/hip/graph_command_buffer.c

+      &command_buffer->hip_graph_nodes[command_buffer->graph_node_count++];
+  size_t dependency_count = command_buffer->hip_barrier_node ? 1 : 0;
+  IREE_ASSERT(dependency_count > 0 &&
+              "Ending a zone should at least depend on the beginning.");


Use IREE_ASSERT_GT and for the error message avoid captialize first char and avoid the end .

antiagainst

Just one final nit. Feel free to land after fixing it.

antiagainst · 2024-04-12T04:50:03Z

runtime/src/iree/hal/drivers/hip/graph_command_buffer.c

-  IREE_ASSERT(dependency_count > 0 &&
-              "Ending a zone should at least depend on the beginning.");
+  IREE_ASSERT_GT(dependency_count, 0,
+                 "Ending a zone should at least depend on the beginning.");


Nit: no captialization and no ending period for assert messages: "end .. beginning".

sogartar · 2024-04-12T15:44:36Z

@antiagainst would you like to also mirror this in the CUDA driver in this PR?

There are some outstanding concurrency issues where some events don't get recorded during collection. If we collect only when the device gets destroyed with a 1s delay, the concurrency issues seem to not manifest.

sogartar · 2024-04-13T02:47:09Z

I rebased and mirrored the change into the CUDA driver.

stellaraccident · 2024-04-13T03:00:46Z

This is great work, Boian. Thank you.

antiagainst · 2024-04-13T21:52:44Z

I rebased and mirrored the change into the CUDA driver.

Nice, thanks! LGTM. Please feel free to land. Make sure you update the commit message title and body accordingly given now this also covers cuda.

) In the HIP and CUDA driver adds tracing with Tracy in graph mode. There are some outstanding concurrency issues where some events don't get recorded during collection. If we collect only when the device gets destroyed with a 1s delay, the concurrency issues seem to not manifest.

sogartar requested review from antiagainst and nithinsubbiah as code owners March 25, 2024 23:54

sogartar mentioned this pull request Mar 26, 2024

[runtime] Concurrency issues in the HIP/CUDA stream/graph drivers #16903

Open

antiagainst requested changes Apr 4, 2024

View reviewed changes

sogartar requested a review from antiagainst April 9, 2024 01:30

antiagainst approved these changes Apr 12, 2024

View reviewed changes

[runtime][hip] Add tracing in graph execution mode

7f6e004

There are some outstanding concurrency issues where some events don't get recorded during collection. If we collect only when the device gets destroyed with a 1s delay, the concurrency issues seem to not manifest.

sogartar force-pushed the hip-graph-tracing branch from 8beb8c7 to e99cbd2 Compare April 12, 2024 23:03

Mirror change to CUDA driver

c419323

sogartar force-pushed the hip-graph-tracing branch from e99cbd2 to c419323 Compare April 13, 2024 02:46

sogartar merged commit 1c49d6a into iree-org:main Apr 15, 2024
53 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[runtime][hip] Add tracing in graph execution mode #16894

[runtime][hip] Add tracing in graph execution mode #16894

sogartar commented Mar 25, 2024

sogartar commented Mar 26, 2024

nithinsubbiah commented Mar 26, 2024

sogartar commented Mar 26, 2024

nithinsubbiah commented Mar 26, 2024

antiagainst Apr 4, 2024

sogartar Apr 9, 2024

antiagainst Apr 4, 2024

antiagainst Apr 4, 2024

sogartar Apr 9, 2024

antiagainst Apr 4, 2024

sogartar Apr 9, 2024

antiagainst Apr 4, 2024

sogartar Apr 9, 2024

antiagainst Apr 4, 2024

sogartar Apr 9, 2024

antiagainst Apr 4, 2024

sogartar Apr 9, 2024

antiagainst Apr 4, 2024

sogartar Apr 9, 2024

antiagainst Apr 4, 2024

sogartar Apr 9, 2024

antiagainst left a comment

antiagainst Apr 12, 2024

sogartar Apr 13, 2024

sogartar commented Apr 12, 2024

sogartar commented Apr 13, 2024

stellaraccident commented Apr 13, 2024

antiagainst commented Apr 13, 2024

[runtime][hip] Add tracing in graph execution mode #16894

[runtime][hip] Add tracing in graph execution mode #16894

Conversation

sogartar commented Mar 25, 2024

sogartar commented Mar 26, 2024

nithinsubbiah commented Mar 26, 2024

sogartar commented Mar 26, 2024

nithinsubbiah commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antiagainst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sogartar commented Apr 12, 2024

sogartar commented Apr 13, 2024

stellaraccident commented Apr 13, 2024

antiagainst commented Apr 13, 2024