From 9d3d5cc07c1ffc0f0d191205ee8fb5440beacf0a Mon Sep 17 00:00:00 2001 From: venkat1361 Date: Tue, 22 Oct 2024 12:19:51 -0500 Subject: [PATCH] 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 --- CHANGELOG.md | 1 + .../rocprofiler-sdk/counters/evaluate_ast.cpp | 8 +- .../counters/tests/evaluate_ast_test.cpp | 99 +++++++++++++++++++ 3 files changed, 102 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 873e51f3b9..3d556603bf 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/source/lib/rocprofiler-sdk/counters/evaluate_ast.cpp b/source/lib/rocprofiler-sdk/counters/evaluate_ast.cpp index 6b092c8a9f..8aa57f5537 100644 --- a/source/lib/rocprofiler-sdk/counters/evaluate_ast.cpp +++ b/source/lib/rocprofiler-sdk/counters/evaluate_ast.cpp @@ -78,7 +78,7 @@ perform_reduction(ReduceOperation reduce_op, std::vectorbegin(), 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(_reduce_op))); diff --git a/source/lib/rocprofiler-sdk/counters/tests/evaluate_ast_test.cpp b/source/lib/rocprofiler-sdk/counters/tests/evaluate_ast_test.cpp index 378df2c9ef..67176b911c 100644 --- a/source/lib/rocprofiler-sdk/counters/tests/evaluate_ast_test.cpp +++ b/source/lib/rocprofiler-sdk/counters/tests/evaluate_ast_test.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -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 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> 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> 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{}) + .first->second.emplace(val, + EvaluateAST({.handle = metric.id()}, metrics, *ast, "gfx9")); + yy_delete_buffer(buf); + delete ast; + } + + std::vector, 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> 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>> 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++; + } + } +}