From c45bf712d195eed335948bace8a173a08d6d8e28 Mon Sep 17 00:00:00 2001
From: Vladimir Indic <139573562+vlaindic@users.noreply.github.com>
Date: Wed, 29 May 2024 12:55:47 +0200
Subject: [PATCH] PC Sampling - blit kernels handling (#878)

* correlation id 0 for blit kernels' samples

* rocprofiler_pc_sampling_snapshot_v1_t docs update (for future use)

* ROCPROFILER_CORRELATION_ID_VALUE_NONE introduced

* Simplifying ROCPROFILER_CORRELATION_ID_VALUE_NONE

* Apply suggestions from code review

Co-authored-by: Vladimir Indic <139573562+vlaindic@users.noreply.github.com>

---------

Co-authored-by: Jonathan R. Madsen <jrmadsen@users.noreply.github.com>
---
 source/include/rocprofiler-sdk/fwd.h                  |  5 +++++
 source/include/rocprofiler-sdk/pc_sampling.h          |  9 +++++++--
 .../lib/rocprofiler-sdk/pc_sampling/hsa_adapter.cpp   |  3 +--
 .../pc_sampling/parser/correlation.hpp                | 11 ++++++-----
 4 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/source/include/rocprofiler-sdk/fwd.h b/source/include/rocprofiler-sdk/fwd.h
index fd8c159b06..781d4ddded 100644
--- a/source/include/rocprofiler-sdk/fwd.h
+++ b/source/include/rocprofiler-sdk/fwd.h
@@ -519,6 +519,11 @@ typedef struct
     rocprofiler_user_data_t external;
 } rocprofiler_correlation_id_t;
 
+/**
+ * @brief The NULL correlation ID value.
+ */
+#define ROCPROFILER_CORRELATION_ID_VALUE_NONE 0ULL
+
 /**
  * @struct rocprofiler_buffer_id_t
  * @brief Buffer ID.
diff --git a/source/include/rocprofiler-sdk/pc_sampling.h b/source/include/rocprofiler-sdk/pc_sampling.h
index 74a6a7252f..e77721c457 100644
--- a/source/include/rocprofiler-sdk/pc_sampling.h
+++ b/source/include/rocprofiler-sdk/pc_sampling.h
@@ -197,7 +197,7 @@ rocprofiler_query_pc_sampling_agent_configurations(
  */
 typedef struct
 {
-    uint8_t valid            : 1;  /// sample is valid
+    uint8_t valid            : 1;  /// ::rocprofiler_pc_sampling_snapshot_v1_t field is valid
     uint8_t type             : 4;
     uint8_t has_stall_reason : 1;
     uint8_t has_wave_cnt     : 1;
@@ -219,7 +219,12 @@ typedef struct
 } rocprofiler_pc_sampling_header_v1_t;
 
 /**
- * @brief TODO: provide the description
+ * @brief For future use.
+ *
+ * @todo: Provide the description
+ * @todo: Should we use bitfields because of C ABI portability?
+ * @todo: Should we abstract this to be architecture agnostic?
+ * @todo: Consider having a query to determine organization of this information.
  */
 typedef struct
 {
diff --git a/source/lib/rocprofiler-sdk/pc_sampling/hsa_adapter.cpp b/source/lib/rocprofiler-sdk/pc_sampling/hsa_adapter.cpp
index decfcb06d2..54d0d6ed51 100644
--- a/source/lib/rocprofiler-sdk/pc_sampling/hsa_adapter.cpp
+++ b/source/lib/rocprofiler-sdk/pc_sampling/hsa_adapter.cpp
@@ -176,8 +176,7 @@ data_ready_callback(void*                                client_callback_data,
 
         if(pcs_parser_status != PCSAMPLE_STATUS_SUCCESS)
         {
-            // TODO: should we end program here or somehow report an error to the user and continue?
-            throw std::runtime_error("Error while parsing PC samples");
+            ROCP_INFO << "PCS Parser encountered samples from a blit kernel.\n";
         }
     });
 }
diff --git a/source/lib/rocprofiler-sdk/pc_sampling/parser/correlation.hpp b/source/lib/rocprofiler-sdk/pc_sampling/parser/correlation.hpp
index 166e6eb10e..0add87bbc9 100644
--- a/source/lib/rocprofiler-sdk/pc_sampling/parser/correlation.hpp
+++ b/source/lib/rocprofiler-sdk/pc_sampling/parser/correlation.hpp
@@ -140,9 +140,9 @@ public:
         if(dev.handle == cache_dev_id && correlation_in == cache_correlation_id_in)
             return cache_correlation_id_out;
 #endif
+        cache_correlation_id_out = dispatch_to_correlation.at({correlation_in, dev});
         cache_dev_id             = dev.handle;
         cache_correlation_id_in  = correlation_in;
-        cache_correlation_id_out = dispatch_to_correlation.at({correlation_in, dev});
         return cache_correlation_id_out;
     }
 
@@ -191,16 +191,17 @@ add_upcoming_samples(const device_handle               device,
     {
         const auto* snap = reinterpret_cast<const perf_sample_snapshot_v1*>(buffer + p);
         samples[p]       = copySample<bHostTrap, GFXIP>((const void*) (buffer + p));
-        samples[p].size  = 0;  // pc sampling record with size 0 will indicate invalid sample
+        samples[p].size  = sizeof(rocprofiler_pc_sampling_record_t);
         try
         {
             Parser::trap_correlation_id_t trap{.raw = snap->correlation_id};
             samples[p].correlation_id = corr_map->get(device, trap);
-            samples[p].size           = sizeof(rocprofiler_pc_sampling_record_t);
-            // set size after corr_map->get which may throw
         } catch(std::exception& e)
         {
-            status = PCSAMPLE_STATUS_PARSER_ERROR;
+            samples[p].correlation_id = {.internal = ROCPROFILER_CORRELATION_ID_VALUE_NONE,
+                                         .external = rocprofiler_user_data_t{
+                                             .value = ROCPROFILER_CORRELATION_ID_VALUE_NONE}};
+            status                    = PCSAMPLE_STATUS_PARSER_ERROR;
         }
     }
     return status;