Max reduction operation bug fix & support reduction for derived counters (#1085)

* Max reduction operation bug fix & support reduction for derived counters

* CHANGELOG

* Added missing new line

---------

Co-authored-by: Gopesh Bhardwaj <gopesh.bhardwaj@amd.com>

CHANGELOG.md

@@ -115,6 +115,7 @@ Full documentation for ROCprofiler-SDK is available at [Click Here](source/docs/
 - Fixed MeanOccupancy* metrics
 - Fix aborted-app validation test to properly check for hipExtHostAlloc command now that it is supported
 - Fix for SQ and GRBM metrics implicitly reduced.
+- Fix Support for derived counters in reduce operation and bug fix for max in reduce
 - Check to force tools to initialize context id with zero.
 - Fix to handle a range of values for select() dimension in expressions parser.
 

source/lib/rocprofiler-sdk/counters/evaluate_ast.cpp

@@ -78,7 +78,7 @@ perform_reduction(ReduceOperation reduce_op, std::vector<rocprofiler_record_coun
         {
             result =
                 *std::max_element(input_array->begin(), input_array->end(), [](auto& a, auto& b) {
-                    return a.counter_value > b.counter_value;
+                    return a.counter_value < b.counter_value;
                 });
             break;
         }
@@ -669,11 +669,7 @@ EvaluateAST::evaluate(
         break;
         case REDUCE_NODE:
         {
-            auto* result = rocprofiler::common::get_val(results_map, _children[0]._metric.id());
-            if(!result)
-                throw std::runtime_error(fmt::format("Unable to lookup results for metric {}",
-                                                     _children[0]._metric.name()));
-
+            auto* result = _children.at(0).evaluate(results_map, cache);
             if(_reduce_op == REDUCE_NONE)
                 throw std::runtime_error(fmt::format("Invalid Second argument to reduce(): {}",
                                                      static_cast<int>(_reduce_op)));

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

@@ -23,6 +23,7 @@
 #include <algorithm>
 #include <cstdint>
 #include <tuple>
+#include <utility>
 
 #include <fmt/core.h>
 #include <gtest/gtest.h>
@@ -1134,3 +1135,101 @@ TEST(evaluate_ast, evaluate_mixed_counters)
         }
     }
 }
+
+TEST(evaluate_ast, derived_counter_reduction)
+{
+    using namespace rocprofiler::counters;
+
+    auto get_base_rec_id = [](uint64_t counter_id) {
+        rocprofiler_counter_instance_id_t base_id = 0;
+        set_counter_in_rec(base_id, {.handle = counter_id});
+        return base_id;
+    };
+
+    auto max_vec = [](auto&& a) -> auto&
+    {
+        a[0].counter_value = std::max_element(a.begin(), a.end(), [](const auto& b, const auto& c) {
+                                 return b.counter_value < c.counter_value;
+                             })->counter_value;
+        a.resize(1);
+        CHECK(a.size() == 1);
+        return a;
+    };
+
+    auto sum_vec = [](auto&& a) -> auto&
+    {
+        for(size_t i = 1; i < a.size(); i++)
+        {
+            a[0].counter_value += a[i].counter_value;
+        }
+        a.resize(1);
+        CHECK(a.size() == 1);
+        return a;
+    };
+
+    std::unordered_map<std::string, Metric> metrics = {
+        {"VOORHEES", Metric("gfx9", "VOORHEES", "a", "a", "a", "", "", 0)},
+        {"KRUEGER", Metric("gfx9", "KRUEGER", "a", "a", "a", "", "", 1)},
+        {"max_BATES",
+         Metric("gfx9", "max_BATES", "C", "C", "C", "reduce(VOORHEES+KRUEGER,max)", "", 2)},
+        {"sum_BATES",
+         Metric("gfx9", "sum_BATES", "C", "C", "C", "reduce(VOORHEES+KRUEGER,sum)", "", 3)}};
+
+    std::unordered_map<std::string, std::vector<rocprofiler_record_counter_t>> base_counter_data = {
+        {"VOORHEES", construct_test_data_dim(get_base_rec_id(0), {ROCPROFILER_DIMENSION_NONE}, 8)},
+        {"KRUEGER", construct_test_data_dim(get_base_rec_id(1), {ROCPROFILER_DIMENSION_NONE}, 8)},
+    };
+
+    std::unordered_map<std::string, std::unordered_map<std::string, EvaluateAST>> asts;
+    for(const auto& [val, metric] : metrics)
+    {
+        RawAST* ast = nullptr;
+        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();
+        asts.emplace("gfx9", std::unordered_map<std::string, EvaluateAST>{})
+            .first->second.emplace(val,
+                                   EvaluateAST({.handle = metric.id()}, metrics, *ast, "gfx9"));
+        yy_delete_buffer(buf);
+        delete ast;
+    }
+
+    std::vector<std::tuple<std::string, std::vector<rocprofiler_record_counter_t>, int64_t>>
+        derived_counters = {
+            {"max_BATES",
+             max_vec(plus_vec(base_counter_data["VOORHEES"], base_counter_data["KRUEGER"])),
+             2},
+            {"sum_BATES",
+             sum_vec(plus_vec(base_counter_data["VOORHEES"], base_counter_data["KRUEGER"])),
+             2},
+        };
+
+    std::unordered_map<uint64_t, std::vector<rocprofiler_record_counter_t>> base_counter_decode;
+    for(const auto& [name, base_counter_v] : base_counter_data)
+    {
+        base_counter_decode[metrics[name].id()] = base_counter_v;
+    }
+
+    for(auto& [name, expected, eval_count] : derived_counters)
+    {
+        ROCP_INFO << name;
+        auto eval_counters =
+            rocprofiler::counters::get_required_hardware_counters(asts, "gfx9", metrics[name]);
+        ASSERT_TRUE(eval_counters);
+        ASSERT_EQ(eval_counters->size(), eval_count);
+        std::vector<std::unique_ptr<std::vector<rocprofiler_record_counter_t>>> cache;
+        asts.at("gfx9").at(name).expand_derived(asts.at("gfx9"));
+        auto ret = asts.at("gfx9").at(name).evaluate(base_counter_decode, cache);
+        EXPECT_EQ(ret->size(), expected.size());
+        int pos = 0;
+        asts.at("gfx9").at(name).set_out_id(*ret);
+        for(const auto& v : *ret)
+        {
+            set_counter_in_rec(expected[pos].id, {.handle = metrics[name].id()});
+            EXPECT_EQ(v.id, expected[pos].id);
+            EXPECT_FLOAT_EQ(v.counter_value, expected[pos].counter_value);
+            pos++;
+        }
+    }
+}