[PATCH] D9009: Value profiling compiler-rt changes

[Betul Buyukkurt via llvm-commits]

betulb added a subscriber: betulb.
betulb added a comment.

> davidxl added inline comments.

> 

> ================

>  Comment at: lib/profile/InstrProfiling.c:81

>  @@ +80,3 @@

>  +void __llvm_profile_instrument_target(uint64_t TargetValue, void *Data_,

>  +  uint32_t CounterIndex) {

>  +

> 

>  ----------------

> 

> Can we make the CounterIndex relative to the value kind? Why is value kind

>  parameter not passed in?


It's not passed in, because it's already relative to the value kind.
CounterIndex is computed inside the InstrProfiling.cpp of the llvm patch
during lowering. It's a value that can be computed in compile time, so
better to do it then.

-Betul

> ================

>  Comment at: lib/profile/InstrProfiling.c:87

>  @@ +86,3 @@

>  +

>  +  if (!Data->ValueCounters) {

>  +    uint64_t NumVSites = 0;

> 

>  ----------------

> 

> This part of the code is cold -- move it out of line into another routine.

> 

> ================

>  Comment at: lib/profile/InstrProfiling.c:119

>  @@ +118,3 @@

>  +

>  +  if (VDataCount >= UCHAR_MAX)

>  +    return;

> 

>  ----------------

> 

> This may still be too large. If VDataCount is this large, the runtime

>  overhead will be substantial.

> 

> In the future, we should enhance it with the following algoithm:

> 

> 1. reserve the first vdata node to do book keeping -- i.e. tracking number of time values get dropped due to too long a list

> 2. When the number of dropping times reaches a threshold, sort the existing linked list to make it in descending order, and zero out the last X entries.

> 

>   Not needed for the first version.

> 

>   ================ Comment at: lib/profile/InstrProfiling.h:33 @@ +32,3 @@ +  VK_FIRST = VK_IndirectCallTarget, +  VK_LAST = VK_IndirectCallTarget +} __llvm_profile_value_kind; ---------------- Make the enumerators name the same as the LLVM counter part -- this allows future refactoring.

> 

> http://reviews.llvm.org/D9009



http://reviews.llvm.org/D9009