[WIP][rocprofiler-sdk] Counters implement variable bit allocation for dimension encoding#4175
Draft
venkat1361 wants to merge 4 commits intodevelopfrom
Draft
[WIP][rocprofiler-sdk] Counters implement variable bit allocation for dimension encoding#4175venkat1361 wants to merge 4 commits intodevelopfrom
venkat1361 wants to merge 4 commits intodevelopfrom
Conversation
Summary: - id_decode.hpp: Added DIM_BIT_LENGTHS[] and DIM_BIT_OFFSETS[] arrays with get_dim_bit_length() and get_dim_bit_offset() helpers. Added static_assert to validate bit allocation. DIMENSION_INSTANCE increased from 6 to 8 bits (256 max value). - evaluate_ast.cpp: Updated perform_reduction(), get_int_encoded_dimensions_from_string(), and perform_selection() to use variable bit allocation. - Test files: Updated to compute per-dimension max values dynamically.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates rocprofiler-sdk’s counter instance ID dimension encoding from a fixed, evenly-split bit layout to a variable per-dimension bit allocation (notably expanding INSTANCE to 10 bits), and adjusts evaluation/tests to match the new layout.
Changes:
- Introduces per-dimension bit length/offset helpers and updates ID set/get logic to use them.
- Updates AST evaluation logic (reduction/selection) and unit tests to use the new per-dimension masks.
- Expands dimension test coverage to iterate up to each dimension’s max representable value.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/id_decode.hpp | Adds variable bit-length/offset tables + helpers; updates dimension encode/decode to use them. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/evaluate_ast.cpp | Updates reduction/selection masking and parsing to use variable bit allocation. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/tests/dimension.cpp | Updates tests to compute max values per-dimension and iterate accordingly. |
| projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/tests/evaluate_ast_test.cpp | Updates test-side masking logic to match the new variable bit allocation. |
Comments suppressed due to low confidence (1)
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/evaluate_ast.cpp:155
perform_reduction()buildsmask_dimfromget_dim_bit_length()/offsetwithout handlingROCPROFILER_DIMENSION_NONE. If the AST specifies DIMENSION_NONE in the reduction list,get_dim_bit_length()returns 0, producingmask_dim == 0and leaving the record id unchanged (i.e., no reduction happens). Either reject DIMENSION_NONE for reduction/select operations or treat it as a special case that clears the full 48-bit dimension field.
// Use variable bit allocation for each dimension
uint64_t dim_bit_length = get_dim_bit_length(dim);
uint64_t dim_bit_offset = get_dim_bit_offset(dim);
int64_t mask_dim = ((1ULL << dim_bit_length) - 1) << dim_bit_offset;
rec.id = rec.id | mask_dim;
rec.id = rec.id ^ mask_dim;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/evaluate_ast.cpp
Outdated
Show resolved
Hide resolved
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/evaluate_ast.cpp
Outdated
Show resolved
Hide resolved
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/tests/dimension.cpp
Outdated
Show resolved
Hide resolved
projects/rocprofiler-sdk/source/lib/rocprofiler-sdk/counters/evaluate_ast.cpp
Outdated
Show resolved
Hide resolved
…nstances - Changed DIMENSION_INSTANCE bit allocation from 8 bits (0-255) to 10 bits (0-1023) - Updated bit layout: 46 bits used, 2 bits reserved - Refactored parse_dimension_selection to use size_t return type - Simplified perform_selection with direct value comparison - Removed unused dim_max_val variable in dimension tests - Used tokenize.hpp utilities for consistent parsing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Some counters have DIMENSION_INSTANCES greater than 64 range, which doesn't fit in current implementation of using 6bits for representing dimension index.
Technical Details
JIRA ID
Test Plan
Test Result
Submission Checklist