Add hierarchical discriminator multiplicity support#257
Conversation
erozenfeld
left a comment
erozenfeld
left a comment
There was a problem hiding this comment.
LGTM with one question
| } | ||
|
|
||
| // Return the full 64-bit offset for 32-bit hierarchical discriminators. | ||
| static constexpr uint64_t GenerateFullOffset(uint64_t Offset) { |
There was a problem hiding this comment.
This is not called from anywhere , is it?
|
Thanks for the review @erozenfeld. I have also added support for aggregating samples with same with same [line + base discriminator] as a second pass as GCC doesn't aggregate. This is only done for create_gcov (for gcc) and supported with a flag. This is also enabled only for gcov version 3. GCC patches are now comitted. |
kugan-nv
force-pushed
the
hd1
branch
3 times, most recently
from
03a7e16 to
28f8a93
Compare
|
@erozenfeld: Ping ? |
|
Is it possible to add some basic tests for this? |
|
Updated the patch with a test. Added a binary and perf.data generated on x86 machine with the gcc 16. The test checks for the hierarchical discriminator processing for GCC. |
snehasish
left a comment
snehasish
left a comment
There was a problem hiding this comment.
lgtm with some minor comments.
|
|
||
| for (i = 0; i < 32; i++) { | ||
| for (j = 0; j < 4; j++) | ||
| sum += a[i + j*4] * i; |
There was a problem hiding this comment.
indentation here is misleading
| float vx[2048]; | ||
| float vy[2048]; | ||
| float vz[2048]; |
There was a problem hiding this comment.
I think if we use this without initialization then the compiler may optimize it away.
| // Whether to use discriminator encoding. | ||
| ABSL_DECLARE_FLAG(bool, use_discriminator_encoding); | ||
|
|
||
| // Whether to use two-pass aggregation for copy_id: |
There was a problem hiding this comment.
note that this is gcov only?
| // Pass 2: Collapse copy_ids before writing (SUM aggregation) | ||
| if (absl::GetFlag(FLAGS_use_two_pass_aggregation)) { | ||
| symbol_map_->CollapseCopyIDs(); | ||
| } |
There was a problem hiding this comment.
I think this is the reason why we have change the profile writer constructors to non-const. Is there a way to preserve the original const contract by refactoring the implementation?
Added support for mutiplicity and copy_id for gcc gcov. This should matches the GCC implementation. I am posting patches to GCC to match this. This patch also adds support for two-pass aggregation when using hierarchical discriminator encoding (gcov_version=3). The two-pass approach correctly handles duplicate samples from code duplication (loop unrolling, etc.). * GetBaseDiscriminator() - Extract bits 0-7 * GetMultiplicity() - Extract bits 8-14 (duplication factor) * GetCopyID() - Extract bits 15-25 (unused but reserved) With the patch we now: 1. Extract multiplicity and copy_id from discriminator 2. Multiply sample count by multiplicity 4. Add samples using copy_id. This was already supported for LLVM.
3 participants
Added support for mutiplicity and copy_id for gcc gcov. This should matches the GCC
implementation. I am posting patches to GCC to match this.
This patch also adds support for two-pass aggregation when using hierarchical
discriminator encoding (gcov_version=3). The two-pass approach correctly
handles duplicate samples from code duplication (loop unrolling, etc.).
With the patch we now: