[SWDEV-518071] Return HSA not loaded status (device counter collection) (#242)

* [SWDEV-518071] Return HSA not loaded status (device counter collection)

This is a state that a caller would want to know about to understand if
they got no counters because of a failure or if they were trying to
collect counters too early (as is the case in the sample, which can
attempt to collect counters before HSA is inited).

* Minor edit

* format

* [SWDEV-518081] Simplify Metric Loading (#243)

* [SWDEV-518071] Return HSA not loaded status (device counter collection)

This is a state that a caller would want to know about to understand if
they got no counters because of a failure or if they were trying to
collect counters too early (as is the case in the sample, which can
attempt to collect counters before HSA is inited).
* [SWDEV-518324] Add AST update support

Allows the ability for ASTs to be updated (instead of an unchangable
static value). Adds a shared pointer return type to protect against
static destructors/modifications from invalidating potentially in use
AST definitions. No functionality/use changes in this PR.
* [SWDEV-518593] Add updatable dimension cache + fix string issues (#252)

* [SWDEV-518593] Add updatable dimension cache + fix string issues

Updates dimension cache to use the same design pattern as AST/Metrics.

Fixes the string scoping issue seen in ASTs, which appears here as well.

* Add rocprofiler_create_counter

Creates derived counters based on input from the API. This PR does three
things:

1. Adds the API + test case
2. Validates that an AST can be constructed from the counter supplied.
3. Updates metrics, ast, and dimension caches to include the new metric.

Metric should be available for use immediately after the call completes.

Due to the regeneration of ASTs, this call should not be performed in
performance sensitive code.

* Suggestion fixes

---------

Co-authored-by: Benjamin Welton <bewelton@amd.com>

* Minor tweak

---------

Co-authored-by: Benjamin Welton <bewelton@amd.com>
Co-authored-by: Venkateshwar Reddy Kandula <vkandula@amd.com>

---------

Co-authored-by: Benjamin Welton <bewelton@amd.com>
Co-authored-by: Venkateshwar Reddy Kandula <vkandula@amd.com>

* Fixes for comments

---------

Co-authored-by: Benjamin Welton <bewelton@amd.com>
Co-authored-by: Kandula, Venkateshwar reddy <Venkateshwarreddy.Kandula@amd.com>
Co-authored-by: Venkateshwar Reddy Kandula <vkandula@amd.com>

---------

Co-authored-by: Benjamin Welton <bewelton@amd.com>
Co-authored-by: Kandula, Venkateshwar reddy <Venkateshwarreddy.Kandula@amd.com>
Co-authored-by: Venkateshwar Reddy Kandula <vkandula@amd.com>

source/lib/rocprofiler-sdk/counters/tests/CMakeLists.txt

@@ -56,8 +56,8 @@ target_link_libraries(
             rocprofiler-sdk::rocprofiler-sdk-hsa-runtime)
 
 set(ROCPROFILER_LIB_COUNTER_TEST_SOURCES
-    metrics_test.cpp evaluate_ast_test.cpp dimension.cpp init_order.cpp core.cpp
-    code_object_loader.cpp device_counting.cpp)
+    metrics_test.cpp evaluate_ast_test.cpp dimension.cpp core.cpp code_object_loader.cpp
+    device_counting.cpp)
 set(ROCPROFILER_LIB_COUNTER_TEST_HEADERS code_object_loader.hpp device_counting.hpp)
 
 add_executable(counter-test)

source/lib/rocprofiler-sdk/counters/tests/core.cpp

@@ -76,17 +76,18 @@ auto
 findDeviceMetrics(const hsa::AgentCache& agent, const std::unordered_set<std::string>& metrics)
 {
     std::vector<counters::Metric> ret;
-    auto                          all_counters = counters::getMetricMap();
+    auto                          mets         = counters::loadMetrics();
+    const auto&                   all_counters = mets->arch_to_metric;
 
     ROCP_INFO << "Looking up counters for " << std::string(agent.name());
-    auto gfx_metrics = common::get_val(*all_counters, std::string(agent.name()));
+    const auto* gfx_metrics = common::get_val(all_counters, std::string(agent.name()));
     if(!gfx_metrics)
     {
         ROCP_ERROR << "No counters found for " << std::string(agent.name());
         return ret;
     }
 
-    for(auto& counter : *gfx_metrics)
+    for(const auto& counter : *gfx_metrics)
     {
         if(metrics.count(counter.name()) > 0 || metrics.empty())
         {
@@ -246,12 +247,13 @@ TEST(core, check_packet_generation)
              * Check that required hardware counters match
              */
             ASSERT_TRUE(profile->agent);
-            auto name_str = std::string(profile->agent->name);
-            auto req_counters =
-                counters::get_required_hardware_counters(counters::get_ast_map(), name_str, metric);
+            auto       name_str     = std::string(profile->agent->name);
+            const auto asts         = counters::get_ast_map();
+            auto       req_counters = counters::get_required_hardware_counters(
+                asts->arch_to_counter_asts, name_str, metric);
             for(const auto& req_metric : *req_counters)
             {
-                if(req_metric.special().empty())
+                if(req_metric.constant().empty())
                 {
                     EXPECT_GT(profile->reqired_hw_counters.count(req_metric), 0)
                         << "Could not find metric - " << req_metric.name();
@@ -811,3 +813,47 @@ TEST_YAML_LOAD:
             2);
     }
 }
+
+TEST(core, create_counter)
+{
+    std::vector<Metric> to_add = {
+        Metric("", "SQ_WAVES_REDUCE_T", "", "", "", "reduce(SQ_WAVES,sum)", "", 10000),
+        Metric("", "SQ_WAVES_AVR_T", "", "", "", "reduce(SQ_WAVES,avr)", "", 10001),
+        Metric("", "SQ_WAVES_INVALID", "", "", "", "reduce(ABC,avr)", "", 10002),
+    };
+
+    ASSERT_EQ(hsa_init(), HSA_STATUS_SUCCESS);
+    test_init();
+
+    registration::init_logging();
+    registration::set_init_status(-1);
+    context::push_client(1);
+    auto agents = hsa::get_queue_controller()->get_supported_agents();
+    for(const auto& [_, agent] : agents)
+    {
+        for(auto& metric : to_add)
+        {
+            rocprofiler_counter_id_t id;
+            auto                     status  = rocprofiler_create_counter(metric.name().c_str(),
+                                                     metric.name().size(),
+                                                     metric.expression().c_str(),
+                                                     metric.expression().size(),
+                                                     metric.description().c_str(),
+                                                     metric.description().size(),
+                                                     agent.get_rocp_agent()->id,
+                                                     &id);
+            auto                     metrics = findDeviceMetrics(agent, {metric.name()});
+            if(metric.name() == "SQ_WAVES_INVALID")
+            {
+                EXPECT_EQ(status, ROCPROFILER_STATUS_ERROR_AST_GENERATION_FAILED);
+                EXPECT_TRUE(metrics.empty());
+            }
+            else
+            {
+                EXPECT_EQ(metrics.size(), 1);
+                EXPECT_EQ(metric.name(), metrics[0].name());
+                EXPECT_EQ(metric.expression(), metrics[0].expression());
+            }
+        }
+    }
+}

source/lib/rocprofiler-sdk/counters/tests/device_counting.cpp

@@ -75,10 +75,10 @@ auto
 findDeviceMetrics(const hsa::AgentCache& agent, const std::unordered_set<std::string>& metrics)
 {
     std::vector<counters::Metric> ret;
-    const auto*                   all_counters = counters::getMetricMap();
-
+    auto                          mets         = counters::loadMetrics();
+    const auto&                   all_counters = mets->arch_to_metric;
     ROCP_INFO << "Looking up counters for " << std::string(agent.name());
-    const auto* gfx_metrics = common::get_val(*all_counters, std::string(agent.name()));
+    const auto* gfx_metrics = common::get_val(all_counters, std::string(agent.name()));
     if(!gfx_metrics)
     {
         ROCP_INFO << "No counters found for " << std::string(agent.name());
@@ -518,8 +518,8 @@ protected:
 
             ROCP_INFO << fmt::format("Running test on agent {:x}",
                                      gpu_agent.get_hsa_agent().handle);
-
-            const auto* agent_map = rocprofiler::common::get_val(counters::get_ast_map(),
+            auto        ast_map   = counters::get_ast_map();
+            const auto* agent_map = rocprofiler::common::get_val(ast_map->arch_to_counter_asts,
                                                                  std::string(gpu_agent.name()));
             CHECK(agent_map);
             const auto* original_ast = rocprofiler::common::get_val(*agent_map, metric_to_test);

source/lib/rocprofiler-sdk/counters/tests/dimension.cpp

@@ -158,17 +158,18 @@ auto
 findDeviceMetrics(const hsa::AgentCache& agent, const std::unordered_set<std::string>& metrics)
 {
     std::vector<counters::Metric> ret;
-    auto                          all_counters = counters::getMetricMap();
+    auto                          mets         = counters::loadMetrics();
+    const auto&                   all_counters = mets->arch_to_metric;
 
     ROCP_INFO << "Looking up counters for " << std::string(agent.name());
-    auto gfx_metrics = common::get_val(*all_counters, std::string(agent.name()));
+    const auto* gfx_metrics = common::get_val(all_counters, std::string(agent.name()));
     if(!gfx_metrics)
     {
         ROCP_ERROR << "No counters found for " << std::string(agent.name());
         return ret;
     }
 
-    for(auto& counter : *gfx_metrics)
+    for(const auto& counter : *gfx_metrics)
     {
         if(metrics.count(counter.name()) > 0 || metrics.empty())
         {
@@ -212,8 +213,8 @@ TEST(dimension, block_dim_test)
              */
             std::unordered_map<counters::rocprofiler_profile_counter_instance_types, uint64_t>
                 rocp_dims;
-            ROCP_INFO << metric.name() << " " << metric.special();
-            if(!metric.special().empty())
+            ROCP_INFO << metric.name() << " " << metric.constant();
+            if(!metric.constant().empty())
             {
                 rocp_dims[counters::rocprofiler_profile_counter_instance_types::
                               ROCPROFILER_DIMENSION_INSTANCE] = 1;
@@ -261,8 +262,8 @@ TEST(dimension, block_dim_test)
             /**
              * Check this value exists in the dimension cache
              */
-            const auto* dim_cache =
-                rocprofiler::common::get_val(counters::get_dimension_cache(), metric.id());
+            auto        dim_ptr   = counters::get_dimension_cache();
+            const auto* dim_cache = rocprofiler::common::get_val(dim_ptr->id_to_dim, metric.id());
             ASSERT_TRUE(dim_cache);
             EXPECT_EQ(fmt::format("{}", fmt::join(dims, "|")),
                       fmt::format("{}", fmt::join(*dim_cache, "|")));

source/lib/rocprofiler-sdk/counters/tests/evaluate_ast_test.cpp

@@ -312,7 +312,7 @@ TEST(evaluate_ast, counter_constants)
         for(const auto& c : *eval_counters)
         {
             EXPECT_NE(expected.find(c.name()), expected.end());
-            EXPECT_TRUE(!c.special().empty());
+            EXPECT_TRUE(!c.constant().empty());
         }
 
         // Check that special counters are being decoded properly by the AST
@@ -451,7 +451,7 @@ TEST(evaluate_ast, evaluate_simple_counters)
     for(const auto& [val, metric] : metrics)
     {
         RawAST* ast = nullptr;
-        auto    buf = yy_scan_string(metric.expression().empty() ? metric.name().c_str()
+        auto*   buf = yy_scan_string(metric.expression().empty() ? metric.name().c_str()
                                                                  : metric.expression().c_str());
         yyparse(&ast);
         ASSERT_TRUE(ast) << metric.expression() << " " << metric.name();
@@ -470,7 +470,7 @@ TEST(evaluate_ast, evaluate_simple_counters)
         ASSERT_TRUE(eval_counters);
         ASSERT_EQ(eval_counters->size(), 1);
         EXPECT_EQ(eval_counters->begin()->name(), name);
-        EXPECT_TRUE(eval_counters->begin()->special().empty());
+        EXPECT_TRUE(eval_counters->begin()->constant().empty());
         std::unordered_map<uint64_t, std::vector<rocprofiler_record_counter_t>> decode = {
             {metrics[name].id(), expected}};
         std::vector<std::unique_ptr<std::vector<rocprofiler_record_counter_t>>> cache;

source/lib/rocprofiler-sdk/counters/tests/init_order.cpp

@@ -1,178 +0,0 @@
-// MIT License
-//
-// Copyright (c) 2023-2025 Advanced Micro Devices, Inc. All rights reserved.
-//
-// Permission is hereby granted, free of charge, to any person obtaining a copy
-// of this software and associated documentation files (the "Software"), to deal
-// in the Software without restriction, including without limitation the rights
-// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
-// copies of the Software, and to permit persons to whom the Software is
-// furnished to do so, subject to the following conditions:
-//
-// The above copyright notice and this permission notice shall be included in all
-// copies or substantial portions of the Software.
-//
-// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
-// SOFTWARE.
-
-#include "lib/common/static_object.hpp"
-#include "lib/common/utility.hpp"
-#include "lib/rocprofiler-sdk/context/context.hpp"
-#include "lib/rocprofiler-sdk/counters/metrics.hpp"
-#include "lib/rocprofiler-sdk/registration.hpp"
-
-#include <rocprofiler-sdk/fwd.h>
-#include <rocprofiler-sdk/registration.h>
-#include <rocprofiler-sdk/rocprofiler.h>
-
-#include <fmt/core.h>
-#include <gtest/gtest.h>
-
-#include <cstdint>
-#include <sstream>
-
-using namespace rocprofiler::counters;
-
-#define ROCPROFILER_CALL(result, msg)                                                              \
-    {                                                                                              \
-        rocprofiler_status_t CHECKSTATUS = result;                                                 \
-        if(CHECKSTATUS != ROCPROFILER_STATUS_SUCCESS)                                              \
-        {                                                                                          \
-            std::string status_msg = rocprofiler_get_status_string(CHECKSTATUS);                   \
-            std::cerr << "[" #result "][" << __FILE__ << ":" << __LINE__ << "] " << msg            \
-                      << " failed with error code " << CHECKSTATUS << ": " << status_msg           \
-                      << std::endl;                                                                \
-            std::stringstream errmsg{};                                                            \
-            errmsg << "[" #result "][" << __FILE__ << ":" << __LINE__ << "] " << msg " failure ("  \
-                   << status_msg << ")";                                                           \
-            throw std::runtime_error(errmsg.str());                                                \
-        }                                                                                          \
-    }
-
-struct metric_map_order
-{
-    metric_map_order() = default;
-    ~metric_map_order() { check_copy(); }
-
-    metric_map_order(const metric_map_order&) = delete;
-    metric_map_order& operator=(const metric_map_order&) = delete;
-
-    metric_map_order(metric_map_order&&) noexcept = delete;
-    metric_map_order& operator=(metric_map_order&&) noexcept = delete;
-
-    void check_copy()
-    {
-        ASSERT_TRUE(!copy_.empty());
-
-        const auto* metricIdMap = rocprofiler::counters::getMetricIdMap();
-        int         fini_status = 0;
-        ROCPROFILER_CALL(rocprofiler_is_finalized(&fini_status), "get finalization state");
-
-        if(fini_status > 0)
-        {
-            // this should only be true in the destructor of the static metric_map_order instance
-            ASSERT_TRUE(metricIdMap != nullptr) << "rocprofiler finalization state: " << fini_status
-                                                << ", metricIdMap: " << metricIdMap;
-
-            // this should ensure the metric id map is destroyed
-            rocprofiler::common::destroy_static_objects();
-            metricIdMap = rocprofiler::counters::getMetricIdMap();
-
-            ASSERT_TRUE(metricIdMap == nullptr) << "rocprofiler finalization state: " << fini_status
-                                                << ", metricIdMap: " << metricIdMap;
-        }
-        else
-        {
-            for(const auto& [id, actual] : copy_)
-            {
-                // Assert because this is getting triggered on shutdown and
-                // we want to fail the test if the values in both maps are not equal.
-                const auto* val = rocprofiler::common::get_val(*metricIdMap, id);
-                ASSERT_TRUE(val != nullptr) << "metricIdMap: " << metricIdMap;
-                ASSERT_TRUE(*val == actual) << "metricIdMap: " << metricIdMap;
-            }
-        }
-    }
-
-private:
-    MetricIdMap copy_ = *CHECK_NOTNULL(rocprofiler::counters::getMetricIdMap());
-};
-
-namespace
-{
-metric_map_order&
-get_metric_map()
-{
-    static metric_map_order order = {};
-    return order;
-}
-
-void
-buffered_callback(rocprofiler_context_id_t,
-                  rocprofiler_buffer_id_t,
-                  rocprofiler_record_header_t**,
-                  size_t,
-                  void*,
-                  uint64_t)
-{}
-
-void
-dispatch_callback(rocprofiler_dispatch_counting_service_data_t,
-                  rocprofiler_profile_config_id_t*,
-                  rocprofiler_user_data_t*,
-                  void*)
-{}
-
-rocprofiler_context_id_t&
-get_client_ctx()
-{
-    static rocprofiler_context_id_t ctx{0};
-    return ctx;
-}
-
-rocprofiler_buffer_id_t&
-get_buffer()
-{
-    static rocprofiler_buffer_id_t buf = {};
-    return buf;
-}
-}  // namespace
-
-// Test that metrics map remains in scope at exit
-TEST(counters_init_order, metric_map_order)
-{
-    rocprofiler::registration::init_logging();
-    // do not call rocprofiler::registration::initialize()!
-    // doing so will add an atexit call which might invoke
-    // rocprofiler::common::destroy_static_objects() before
-    // the get_metric_map() instance is destroyed
-
-    rocprofiler::registration::set_init_status(-1);
-    rocprofiler::context::push_client(1);
-    ROCPROFILER_CALL(rocprofiler_create_context(&get_client_ctx()), "context creation failed");
-    ROCPROFILER_CALL(rocprofiler_create_buffer(get_client_ctx(),
-                                               4096,
-                                               2048,
-                                               ROCPROFILER_BUFFER_POLICY_LOSSLESS,
-                                               buffered_callback,
-                                               nullptr,
-                                               &get_buffer()),
-                     "buffer creation failed");
-    ROCPROFILER_CALL(rocprofiler_configure_buffered_dispatch_counting_service(
-                         get_client_ctx(), get_buffer(), dispatch_callback, nullptr),
-                     "Could not setup buffered service");
-    rocprofiler::registration::set_init_status(1);
-
-    auto& global_metric_map = get_metric_map();
-    global_metric_map.check_copy();
-
-    auto local_metric_map = metric_map_order{};
-    local_metric_map.check_copy();
-
-    rocprofiler::registration::finalize();
-}

source/lib/rocprofiler-sdk/counters/tests/metrics_test.cpp

@@ -61,7 +61,21 @@ loadTestData(const std::unordered_map<std::string, std::vector<std::vector<std::
 
 TEST(metrics, base_load)
 {
-    auto rocp_data = counters::getBaseHardwareMetrics();
+    auto loaded_metrics = counters::loadMetrics();
+    auto rocp_data      = [&]() {
+        // get only base metrics (those without expressions)
+        std::unordered_map<std::string, std::vector<counters::Metric>> ret;
+        for(const auto& [gfx, metrics] : loaded_metrics->arch_to_metric)
+        {
+            std::vector<counters::Metric> base_metrics;
+            std::copy_if(metrics.begin(),
+                         metrics.end(),
+                         std::back_inserter(base_metrics),
+                         [](const auto& m) { return m.expression().empty(); });
+            if(!base_metrics.empty()) ret.emplace(gfx, std::move(base_metrics));
+        }
+        return ret;
+    }();
     auto test_data = loadTestData(basic_gfx908);
 
     ASSERT_EQ(rocp_data.count("gfx908"), 1);
@@ -97,7 +111,22 @@ TEST(metrics, base_load)
 
 TEST(metrics, derived_load)
 {
-    auto rocp_data = counters::getDerivedHardwareMetrics();
+    auto loaded_metrics = counters::loadMetrics();
+    auto rocp_data      = [&]() {
+        // get only derrived metrics
+        std::unordered_map<std::string, std::vector<counters::Metric>> ret;
+        for(const auto& [gfx, metrics] : loaded_metrics->arch_to_metric)
+        {
+            std::vector<counters::Metric> derived_metrics;
+            std::copy_if(metrics.begin(),
+                         metrics.end(),
+                         std::back_inserter(derived_metrics),
+                         [](const auto& m) { return !m.expression().empty(); });
+            if(!derived_metrics.empty()) ret.emplace(gfx, std::move(derived_metrics));
+        }
+        return ret;
+    }();
+
     auto test_data = loadTestData(derived_gfx908);
     ASSERT_EQ(rocp_data.count("gfx908"), 1);
     ASSERT_EQ(test_data.count("gfx908"), 1);
@@ -128,8 +157,10 @@ TEST(metrics, derived_load)
 
 TEST(metrics, check_agent_valid)
 {
-    const auto& rocp_data      = *counters::getMetricMap();
-    auto        common_metrics = [&]() -> std::set<uint64_t> {
+    auto        mets      = counters::loadMetrics();
+    const auto& rocp_data = mets->arch_to_metric;
+
+    auto common_metrics = [&]() -> std::set<uint64_t> {
         std::set<uint64_t> ret;
         for(const auto& [gfx, counters] : rocp_data)
         {
@@ -171,7 +202,7 @@ TEST(metrics, check_agent_valid)
             if(other_gfx == gfx) continue;
             for(const auto& metric : other_counters)
             {
-                if(common_metrics.count(metric.id()) || !metric.special().empty()) continue;
+                if(common_metrics.count(metric.id()) > 0 || !metric.constant().empty()) continue;
                 EXPECT_EQ(counters::checkValidMetric(gfx, metric), false)
                     << fmt::format("GFX {} has Metric {} but shouldn't", gfx, metric);
             }
@@ -181,8 +212,9 @@ TEST(metrics, check_agent_valid)
 
 TEST(metrics, check_public_api_query)
 {
-    const auto* id_map = counters::getMetricIdMap();
-    for(const auto& [id, metric] : *id_map)
+    auto        metrics_map = rocprofiler::counters::loadMetrics();
+    const auto& id_map      = metrics_map->id_to_metric;
+    for(const auto& [id, metric] : id_map)
     {
         rocprofiler_counter_info_v0_t version;
 
@@ -190,10 +222,10 @@ TEST(metrics, check_public_api_query)
             rocprofiler_query_counter_info(
                 {.handle = id}, ROCPROFILER_COUNTER_INFO_VERSION_0, static_cast<void*>(&version)),
             ROCPROFILER_STATUS_SUCCESS);
-        EXPECT_EQ(version.name, metric.name().c_str());
-        EXPECT_EQ(version.block, metric.block().c_str());
-        EXPECT_EQ(version.expression, metric.expression().c_str());
+        EXPECT_EQ(std::string(version.name), metric.name());
+        EXPECT_EQ(std::string(version.block), metric.block());
+        EXPECT_EQ(std::string(version.expression), metric.expression());
         EXPECT_EQ(version.is_derived, !metric.expression().empty());
-        EXPECT_EQ(version.description, metric.description().c_str());
+        EXPECT_EQ(std::string(version.description), metric.description());
     }
 }