Skip to content

Fix CPX mode crash by removing agent encoding from counter IDs #2522

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
usrihari123 wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Fix CPX mode crash by removing agent encoding from counter IDs #2522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 45 additions & 40 deletions projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,33 +88,40 @@ get_static_ptr_array(const std::vector<Tp>& vec)

} // namespace

// Helper function to extract agent_id from agent-encoded counter_id
// Returns agent_id with handle=0 if not found
rocprofiler_agent_id_t
get_agent_id_from_counter_id(rocprofiler_counter_id_t counter_id)
{
// Extract logical_node_id from counter ID encoding
auto agent_index = get_agent_from_counter_id(counter_id) - AGENT_ENCODING_OFFSET;

// Find the agent with matching logical_node_id
for(const auto* agent : rocprofiler::agent::get_agents())
{
if(agent && agent->logical_node_id == agent_index)
{
return agent->id;
}
}

// Return invalid agent_id if not found
return sdk::null_agent_id;
}

} // namespace counters
} // namespace rocprofiler

namespace counters = ::rocprofiler::counters;
namespace common = ::rocprofiler::common;

namespace
{
// Global mapping from base_metric_id to agent_id for counters
// This replaces the removed agent encoding in counter IDs
// Populated when rocprofiler_iterate_agent_supported_counters() is called
std::unordered_map<uint64_t, rocprofiler_agent_id_t>&
get_counter_agent_map()
{
static auto* map = new std::unordered_map<uint64_t, rocprofiler_agent_id_t>{};
return *map;
}
Comment on lines +102 to +107
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The global mapping from base_metric_id to agent_id has a critical design flaw: when multiple agents share the same architecture (e.g., two GPUs with gfx90a), they will have identical metric IDs. When rocprofiler_iterate_agent_supported_counters is called for each agent, the second call will overwrite the mapping from the first call, causing get_agent_for_counter to always return the last agent that was iterated. This breaks multi-GPU systems where agents share the same architecture.

Additionally, this unordered_map is not thread-safe. Concurrent writes from multiple threads calling rocprofiler_iterate_agent_supported_counters or concurrent read/write access will cause undefined behavior. Consider using a Synchronized wrapper like the pattern at line 67, or using a different approach that doesn't rely on a single global agent per metric ID (since metric IDs are architecture-based, not agent-specific).

Copilot uses AI. Check for mistakes.

// Helper to get agent_id for a counter_id (replaces get_agent_id_from_counter_id)
rocprofiler_agent_id_t
get_agent_for_counter(rocprofiler_counter_id_t counter_id)
{
auto base_metric_id = counters::get_base_metric_from_counter_id(counter_id);
auto& map = get_counter_agent_map();

if(auto it = map.find(base_metric_id); it != map.end())
{
return it->second;
}

Comment on lines +115 to +120
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 115 and 120 contain trailing whitespace. Consider removing it to maintain code cleanliness and avoid unnecessary diff noise in version control.

Suggested change
if(auto it = map.find(base_metric_id); it != map.end())
{
return it->second;
}
if(auto it = map.find(base_metric_id); it != map.end())
{
return it->second;
}

Copilot uses AI. Check for mistakes.
return rocprofiler::sdk::null_agent_id;
}
} // namespace

extern "C" {
/**
* @brief Query Counter info such as name or description.
Expand Down Expand Up @@ -188,10 +195,9 @@ rocprofiler_query_counter_info(rocprofiler_counter_id_t counter_id,

// Construct all possible permutations of instance ids. This is every instance
// that can be returned by the counter across all dimensions.
// Note: Dimensions are agent-specific. This function uses the agent from counter ID encoding.
auto dim_permutations = [&](auto& out_struct) {
// Get agent from counter ID encoding
auto agent_id = counters::get_agent_id_from_counter_id(counter_id);
// Get agent from counter-agent mapping
auto agent_id = get_agent_for_counter(counter_id);
if(agent_id == rocprofiler::sdk::null_agent_id) return false;

auto dim_ptr = counters::get_dimension_cache(agent_id);
Expand Down Expand Up @@ -297,8 +303,8 @@ rocprofiler_query_counter_info(rocprofiler_counter_id_t counter_id,

if(!base_info(_out_struct)) return ROCPROFILER_STATUS_ERROR_COUNTER_NOT_FOUND;

// Get agent from counter ID encoding
auto agent_id = counters::get_agent_id_from_counter_id(counter_id);
// Get agent from counter-agent mapping
auto agent_id = get_agent_for_counter(counter_id);
if(agent_id.handle == 0) return ROCPROFILER_STATUS_ERROR_AGENT_NOT_FOUND;

if(!dim_info(_out_struct, agent_id)) return ROCPROFILER_STATUS_ERROR_DIM_NOT_FOUND;
Expand Down Expand Up @@ -367,14 +373,21 @@ rocprofiler_iterate_agent_supported_counters(rocprofiler_agent_id_t
auto metrics = counters::getMetricsForAgent(agent);
if(metrics.empty()) return ROCPROFILER_STATUS_ERROR_AGENT_ARCH_NOT_SUPPORTED;

// Store mapping from base_metric_id to agent_id for later queries
auto& counter_agent_map = get_counter_agent_map();

std::vector<rocprofiler_counter_id_t> ids;
ids.reserve(metrics.size());
for(const auto& metric : metrics)
{
// Create agent-encoded counter ID using the agent's logical_node_id
// Counter ID contains only the base metric ID.
// Agent information is provided via the agent_id callback parameter.
rocprofiler_counter_id_t counter_id{.handle = 0};
counters::set_base_metric_in_counter_id(counter_id, metric.id());
counters::set_agent_in_counter_id(counter_id, agent->logical_node_id);

// Store mapping from base metric ID to agent for later lookup
counter_agent_map[metric.id()] = agent_id;

Comment on lines +387 to +390
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mapping assumes each metric ID uniquely identifies an agent, but metric IDs are architecture-based. When this function is called for multiple agents with the same architecture (e.g., two gfx90a GPUs), the loop will overwrite the mapping for each metric_id, storing only the last agent. This breaks any subsequent calls to rocprofiler_query_counter_info or rocprofiler_iterate_counter_dimensions, which will retrieve the wrong agent information for counters from earlier agents with the same architecture.

Suggested change
// Store mapping from base metric ID to agent for later lookup
counter_agent_map[metric.id()] = agent_id;
// Store mapping from base metric ID to agent for later lookup.
// Avoid overwriting an existing mapping when multiple agents share
// the same architecture-based metric ID.
if(counter_agent_map.find(metric.id()) == counter_agent_map.end())
{
counter_agent_map[metric.id()] = agent_id;
}

Copilot uses AI. Check for mistakes.
Comment on lines +387 to +390
Copy link

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 387 and 390 contain trailing whitespace. Consider removing it to maintain code cleanliness and avoid unnecessary diff noise in version control.

Suggested change
// Store mapping from base metric ID to agent for later lookup
counter_agent_map[metric.id()] = agent_id;
// Store mapping from base metric ID to agent for later lookup
counter_agent_map[metric.id()] = agent_id;

Copilot uses AI. Check for mistakes.
ids.push_back(counter_id);
}

Expand All @@ -395,17 +408,9 @@ rocprofiler_query_record_counter_id(rocprofiler_counter_instance_id_t id,
// Get base metric ID from instance record (bits 63-48)
uint16_t base_metric = static_cast<uint16_t>(id >> counters::DIM_BIT_LENGTH);

// Try to get agent encoding from ROCPROFILER_DIMENSION_AGENT dimension field
uint8_t agent_encoded =
static_cast<uint8_t>(counters::rec_to_dim_pos(id, counters::ROCPROFILER_DIMENSION_AGENT));

// Reconstruct full agent-encoded counter ID
// Note: agent_encoded includes the offset, but set_agent_in_counter_id() adds the offset,
// so we need to subtract it first to get the raw logical_node_id
// Agent encoding removed - only reconstruct base metric ID
counter_id->handle = 0;
counters::set_base_metric_in_counter_id(*counter_id, base_metric);
counters::set_agent_in_counter_id(
*counter_id, agent_encoded > 0 ? agent_encoded - counters::AGENT_ENCODING_OFFSET : 0);

return ROCPROFILER_STATUS_SUCCESS;
}
Expand All @@ -425,11 +430,11 @@ rocprofiler_iterate_counter_dimensions(rocprofiler_counter_id_t id,
rocprofiler_available_dimensions_cb_t info_cb,
void* user_data)
{
// Extract base metric ID from counter_id (handles agent-encoded counter IDs)
// Extract base metric ID from counter_id
auto base_metric_id = counters::get_base_metric_from_counter_id(id);

// Get agent from counter ID encoding
auto agent_id = counters::get_agent_id_from_counter_id(id);
// Get agent from counter-agent mapping
auto agent_id = get_agent_for_counter(id);
if(agent_id.handle == 0) return ROCPROFILER_STATUS_ERROR_AGENT_NOT_FOUND;

auto dim_ptr = counters::get_dimension_cache(agent_id);
Expand Down
14 changes: 4 additions & 10 deletions projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/evaluate_ast.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,10 +735,8 @@ EvaluateAST::read_special_counters(
if(!out_map[metric.id()].empty()) out_map[metric.id()].clear();
auto& record = out_map[metric.id()].emplace_back();
set_counter_in_rec(record.id, {.handle = metric.id()});
// Don't use DIMENSION_NONE as it overwrites the DIMENSION_AGENT field
// Instead, explicitly set DIMENSION_AGENT with the agent's logical_node_id
set_dim_in_rec(
record.id, ROCPROFILER_DIMENSION_AGENT, agent.logical_node_id + AGENT_ENCODING_OFFSET);
// Agent encoding removed - DIMENSION_AGENT set by set_counter_in_rec to 0
// Agent info available from record.agent_id field

record.counter_value = get_agent_property(metric.name(), agent);
}
Expand Down Expand Up @@ -778,12 +776,8 @@ EvaluateAST::read_pkt(const aql::CounterPacketConstruct* pkt_gen, hsa::AQLPacket
CHECK_EQ(aql_status, ROCPROFILER_STATUS_SUCCESS)
<< rocprofiler_get_status_string(aql_status);

// Set DIMENSION_AGENT with the agent's logical_node_id
auto agent_id = it.pkt_gen->agent();
const auto* agent = CHECK_NOTNULL(rocprofiler::agent::get_agent(agent_id));
set_dim_in_rec(next_rec.id,
ROCPROFILER_DIMENSION_AGENT,
agent->logical_node_id + AGENT_ENCODING_OFFSET);
// Agent encoding removed - DIMENSION_AGENT set by set_counter_in_rec to 0
// Agent info available from record.agent_id field

// set_dim_in_rec(next_rec.id, ROCPROFILER_DIMENSION_NONE, vec.size() - 1);
// Note: in the near future we need to use hw_counter here instead
Expand Down
33 changes: 1 addition & 32 deletions projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/id_decode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,31 +89,7 @@ aqlprofile_id_to_rocprof_instance()
return *aql_to_rocprof_dims;
}

// Counter ID encoding/decoding implementations
void
set_agent_in_counter_id(rocprofiler_counter_id_t& id, uint8_t agent_logical_node_id)
{
// Check that logical_node_id + offset fits in 6 bits
// With AGENT_ENCODING_OFFSET=1, this allows logical_node_id 0-62 (63 agents)
CHECK(agent_logical_node_id < ((1 << AGENT_BIT_LENGTH) - AGENT_ENCODING_OFFSET))
<< "Agent logical_node_id " << static_cast<int>(agent_logical_node_id)
<< " exceeds limit (max " << ((1 << AGENT_BIT_LENGTH) - AGENT_ENCODING_OFFSET - 1)
<< " to allow for encoding offset)";

// Add encoding offset to ensure agent 0 is detectable (non-zero)
uint8_t agent_encoded = agent_logical_node_id + AGENT_ENCODING_OFFSET;

// Clear agent bits and set new value
id.handle = (id.handle & ~(AGENT_MASK << AGENT_BIT_OFFSET)) |
(static_cast<uint64_t>(agent_encoded) << AGENT_BIT_OFFSET);
}

uint8_t
get_agent_from_counter_id(rocprofiler_counter_id_t id)
{
return static_cast<uint8_t>((id.handle >> AGENT_BIT_OFFSET) & AGENT_MASK);
}

// Counter ID encoding/decoding implementations (agent encoding removed)
void
set_base_metric_in_counter_id(rocprofiler_counter_id_t& id, uint16_t metric_id)
{
Expand All @@ -128,12 +104,5 @@ get_base_metric_from_counter_id(rocprofiler_counter_id_t id)
return static_cast<uint16_t>(id.handle & BASE_METRIC_MASK);
}

bool
is_agent_encoded_counter_id(rocprofiler_counter_id_t id)
{
// Check if agent bits are non-zero
return ((id.handle >> AGENT_BIT_OFFSET) & AGENT_MASK) != 0;
}

} // namespace counters
} // namespace rocprofiler
63 changes: 19 additions & 44 deletions projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/id_decode.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,25 +70,15 @@ inline size_t
rec_to_dim_pos(rocprofiler_counter_instance_id_t id,
rocprofiler_profile_counter_instance_types dim);

// Counter ID encoding/decoding functions for agent-specific counter IDs
void
set_agent_in_counter_id(rocprofiler_counter_id_t& id, uint8_t agent_logical_node_id);
uint8_t
get_agent_from_counter_id(rocprofiler_counter_id_t id);
// Counter ID encoding/decoding functions (agent encoding removed)
void
set_base_metric_in_counter_id(rocprofiler_counter_id_t& id, uint16_t metric_id);
uint16_t
get_base_metric_from_counter_id(rocprofiler_counter_id_t id);
bool
is_agent_encoded_counter_id(rocprofiler_counter_id_t id);

// Counter ID encoding constants
constexpr uint64_t AGENT_BIT_OFFSET = 32;
constexpr uint64_t AGENT_BIT_LENGTH = 6;
constexpr uint64_t BASE_METRIC_BIT_LENGTH = 16;
constexpr uint64_t AGENT_MASK = ((1ULL << AGENT_BIT_LENGTH) - 1);
constexpr uint64_t BASE_METRIC_MASK = ((1ULL << BASE_METRIC_BIT_LENGTH) - 1);
constexpr uint8_t AGENT_ENCODING_OFFSET = 1; // Offset to reserve 0 for detection

const std::unordered_map<int, rocprofiler_profile_counter_instance_types>&
aqlprofile_id_to_rocprof_instance();
Expand All @@ -102,16 +92,9 @@ rocprofiler::counters::rec_to_counter_id(rocprofiler_counter_instance_id_t id)
// Extract base metric ID from instance record (bits 63-48)
uint16_t base_metric = static_cast<uint16_t>(id >> DIM_BIT_LENGTH);

// Extract agent encoding from ROCPROFILER_DIMENSION_AGENT dimension field
uint8_t agent_encoded = static_cast<uint8_t>(rec_to_dim_pos(id, ROCPROFILER_DIMENSION_AGENT));

// Reconstruct full agent-encoded counter ID
// Note: agent_encoded includes the offset, but set_agent_in_counter_id() adds the offset,
// so we need to subtract it first to get the raw logical_node_id
// Agent encoding removed - only reconstruct base metric ID
rocprofiler_counter_id_t counter_id{.handle = 0};
set_base_metric_in_counter_id(counter_id, base_metric);
set_agent_in_counter_id(counter_id,
agent_encoded > 0 ? agent_encoded - AGENT_ENCODING_OFFSET : 0);

return counter_id;
}
Expand Down Expand Up @@ -159,14 +142,9 @@ rocprofiler::counters::set_counter_in_rec(rocprofiler_counter_instance_id_t& id,
// Set the base metric ID in bits 63-48
id = id | (static_cast<uint64_t>(base_metric) << DIM_BIT_LENGTH);

// Store agent encoding in ROCPROFILER_DIMENSION_AGENT dimension field
// NOTE: ROCPROFILER_DIMENSION_AGENT is a special dimension used to store agent information
// This field is for internal use only and should not be set by external code
uint8_t agent_encoded = get_agent_from_counter_id(value);

// Unconditionally set DIMENSION_AGENT, even if agent_encoded is 0
// (This preserves the agent encoding for all counter IDs)
set_dim_in_rec(id, ROCPROFILER_DIMENSION_AGENT, agent_encoded);
// Agent encoding has been removed - DIMENSION_AGENT always set to 0
// Agent information is obtained from context (callback parameters, record fields, etc.)
set_dim_in_rec(id, ROCPROFILER_DIMENSION_AGENT, 0);
}

size_t
Expand All @@ -184,26 +162,23 @@ rocprofiler::counters::rec_to_dim_pos(rocprofiler_counter_instance_id_t
return id >> ((dim - 1) * bit_length);
}

// Counter ID encoding/decoding implementations
//
// NEW COUNTER ID REPRESENTATION (Agent-Specific Counter IDs):
// ============================================================
// Counter IDs (rocprofiler_counter_id_t::handle, 64-bit) are now agent-specific.
// The counter ID encodes both the base metric ID and the agent's logical_node_id.
// Counter ID Representation (Agent Encoding Removed):
// ====================================================
// Counter IDs (rocprofiler_counter_id_t::handle, 64-bit) contain only the base metric ID.
// Agent encoding has been removed to eliminate the 63-agent limit that caused failures
// in CPX mode and multi-GPU systems.
//
// Bit Layout:
// Bits 63-38: Reserved/unused (26 bits)
// Bits 37-32: Agent logical_node_id (6 bits) - supports up to 64 agents
// Bits 31-16: Reserved/unused (16 bits)
// Bits 63-16: Reserved/unused (48 bits)
// Bits 15-0: Base metric ID (16 bits) - architecture-based metric identifier
//
// Rationale:
// - Allows unique counter IDs for agents with same architecture but different configurations
// (e.g., same gfx90a but different CU counts: 110 vs 104)
// - Maintains consistency: counter IDs are now agent-specific, matching agent-specific dimensions
// - Agent encoding is mandatory: All counter IDs must have agent encoding (agent bits != 0)
// Agent Information Sources:
// - rocprofiler_iterate_agent_supported_counters() callback receives agent_id parameter
// - rocprofiler_counter_record_t.agent_id field in counter records
// - rocprofiler_dispatch_counting_service_record_t.dispatch_info.agent_id
// - Internal mapping (base_metric_id → agent_id) for dimension queries
//
// Usage:
// - use set_agent_in_counter_id() / set_base_metric_in_counter_id() to encode
// - use get_agent_from_counter_id() / get_base_metric_from_counter_id() to decode
// - use is_agent_encoded_counter_id() to check if counter ID has agent encoding
// - set_base_metric_in_counter_id() to set metric ID
// - get_base_metric_from_counter_id() to extract metric ID
// - Agent info always available from context, never needs extraction from counter_id
16 changes: 3 additions & 13 deletions projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/metrics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,14 +373,9 @@ getMetricsForAgent(const rocprofiler_agent_t* agent)
{
Metric agent_metric = base_metric;

// Encode agent into counter ID using agent's logical_node_id + AGENT_ENCODING_OFFSET.
// We add offset so that agent 0 has non-zero encoding and is detectable as
// agent-encoded. Only logical_node_id values 0-62 (i.e., 63 agents) are supported,
// since adding AGENT_ENCODING_OFFSET (1) results in encoded values 1-63, which fit in a
// 6-bit field.
// Agent encoding removed - only store base metric ID
rocprofiler_counter_id_t new_id{.handle = 0};
set_base_metric_in_counter_id(new_id, base_metric.id());
set_agent_in_counter_id(new_id, static_cast<uint8_t>(agent->logical_node_id));

agent_metric.set_id(new_id.handle);
agent_specific_metrics.push_back(agent_metric);
Expand All @@ -398,13 +393,8 @@ checkValidMetric(const std::string& agent, const Metric& metric)
auto metrics = loadMetrics();
const auto* agent_map = common::get_val(metrics->arch_to_id, agent);

// Extract base metric ID if counter ID is agent-encoded
rocprofiler_counter_id_t counter_id{.handle = metric.id()};
uint64_t base_metric_id = is_agent_encoded_counter_id(counter_id)
? get_base_metric_from_counter_id(counter_id)
: metric.id();

return agent_map != nullptr && agent_map->count(base_metric_id) > 0;
// Agent encoding has been removed - metric.id() is always the base metric ID
return agent_map != nullptr && agent_map->count(metric.id()) > 0;
}

bool
Expand Down
9 changes: 2 additions & 7 deletions projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/sample_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,13 +97,8 @@ proccess_completed_cb(completed_cb_params_t&& params)
ast.set_out_id(*ret);

// Ensure all output records have proper agent encoding in DIMENSION_AGENT
for(auto& val : *ret)
{
counters::set_dim_in_rec(
val.id,
counters::ROCPROFILER_DIMENSION_AGENT,
prof_config->agent->logical_node_id + counters::AGENT_ENCODING_OFFSET);
}
// Agent encoding removed - DIMENSION_AGENT already set to 0 by set_counter_in_rec
// Agent info available from record.agent_id field

out.reserve(out.size() + ret->size());
for(auto& val : *ret)
Expand Down
Loading