[PATCH] Indirect call target profiling compiler-rt changes
Betul Buyukkurt
betulb at codeaurora.org
Thu May 28 15:20:48 PDT 2015
> Betul Buyukkurt <betulb at codeaurora.org> writes:
>> New fields are reserved in enum ValueKind { ... } for future value
>> profiling types.
>>
>>
>> http://reviews.llvm.org/D9009
>>
>> Files:
>> lib/profile/InstrProfiling.c
>> lib/profile/InstrProfiling.h
>> lib/profile/InstrProfilingBuffer.c
>> lib/profile/InstrProfilingFile.c
>>
>> EMAIL PREFERENCES
>> http://reviews.llvm.org/settings/panel/emailpreferences/
>>
>> Index: lib/profile/InstrProfiling.c
>> ===================================================================
>> --- lib/profile/InstrProfiling.c
>> +++ lib/profile/InstrProfiling.c
>> @@ -8,6 +8,7 @@
>> \*===----------------------------------------------------------------------===*/
>>
>> #include "InstrProfiling.h"
>> +#include <stdlib.h>
>> #include <string.h>
>>
>> __attribute__((visibility("hidden")))
>> @@ -24,7 +25,7 @@
>> unsigned char R = sizeof(void *) == sizeof(uint64_t) ? 'r' : 'R';
>> return
>> (uint64_t)255 << 56 |
>> - (uint64_t)'l' << 48 |
>> + (uint64_t)'v' << 48 |
>> (uint64_t)'p' << 40 |
>> (uint64_t)'r' << 32 |
>> (uint64_t)'o' << 24 |
>
> Not necessary.
Reverted. Done.
>
>> @@ -36,13 +37,154 @@
>> __attribute__((visibility("hidden")))
>> uint64_t __llvm_profile_get_version(void) {
>> /* This should be bumped any time the output format changes. */
>> - return 1;
>> + return 2;
>> }
>>
>> __attribute__((visibility("hidden")))
>> void __llvm_profile_reset_counters(void) {
>> uint64_t *I = __llvm_profile_begin_counters();
>> uint64_t *E = __llvm_profile_end_counters();
>>
>> memset(I, 0, sizeof(uint64_t)*(E - I));
>> +
>> + const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();
>> + const __llvm_profile_data *DataEnd = __llvm_profile_end_data();
>> + for (const __llvm_profile_data *DI = DataBegin; DI != DataEnd; ++DI)
>> {
>> +
>> + for (uint32_t VKI = VK_FIRST; VKI != VK_LAST; ++VKI) {
>> +
>> + if (!DI->ValueCounters[VKI])
>> + continue;
>> +
>> + for (uint32_t i = 0; i < DI->NumValueSites[VKI]; ++i) {
>> + __llvm_profile_value_data *CurrentValueData =
>> DI->ValueCounters[VKI][i];
>> +
>> + while (CurrentValueData) {
>> + CurrentValueData->NumTaken = 0;
>> + CurrentValueData = CurrentValueData->Next;
>> + }
>> + }
>> + }
>> + }
>> +}
>> +
>> +__attribute__((visibility("hidden")))
>> +void __llvm_profile_instrument_target(uint32_t ValueKind, uint64_t
>> TargetValue,
>> + void *Data_, uint32_t CounterIndex) {
>> +
>> + __llvm_profile_data *Data = (__llvm_profile_data*)Data_;
>> + if (!Data)
>> + return;
>> +
>> + if (0 >= Data->NumValueSites[ValueKind])
>> + return;
>> +
>> + if (CounterIndex >= Data->NumValueSites[ValueKind])
>> + return;
>> +
>> + if (!Data->ValueCounters[ValueKind]) {
>> + __llvm_profile_value_data** Mem = (__llvm_profile_value_data**)
>> + calloc(Data->NumValueSites[ValueKind],
>> sizeof(__llvm_profile_value_data*));
>> + if (!Mem)
>> + return;
>> + if (!__sync_bool_compare_and_swap(
>> + &(Data->ValueCounters[ValueKind]), 0, Mem)) {
>> + free(Mem);
>> + return;
>> + }
>> + }
>> +
>> + __llvm_profile_value_data *PrevValueData = NULL;
>> + __llvm_profile_value_data *CurrentValueData =
>> + Data->ValueCounters[ValueKind][CounterIndex];
>> +
>> + while (CurrentValueData) {
>> + if (TargetValue == CurrentValueData->TargetValue) {
>> + CurrentValueData->NumTaken++;
>> + return;
>> + }
>> + PrevValueData = CurrentValueData;
>> + CurrentValueData = CurrentValueData->Next;
>> + }
>> +
>> + CurrentValueData = (__llvm_profile_value_data*)
>> + calloc(1, sizeof(__llvm_profile_value_data));
>> + if (!CurrentValueData)
>> + return;
>> +
>> + CurrentValueData->ValueKind = ValueKind;
>> + CurrentValueData->TargetValue = TargetValue;
>> + CurrentValueData->CounterIndex = CounterIndex;
>> + CurrentValueData->NumTaken++;
>> +
>> + uint32_t Success = 0;
>> + if (!Data->ValueCounters[ValueKind][CounterIndex])
>> + Success = __sync_bool_compare_and_swap(
>> + &(Data->ValueCounters[ValueKind][CounterIndex]), 0,
>> CurrentValueData);
>> + else if (PrevValueData && !PrevValueData->Next)
>> + Success = __sync_bool_compare_and_swap(
>> + &(PrevValueData->Next), 0, CurrentValueData);
>> +
>> + if (!Success) {
>> + free(CurrentValueData);
>> + return;
>> + }
>> +
>> + TotalValueDataCount += Success;
>
> Do we need to update this count atomically?
Done.
>
>> }
>> +
>> +__attribute__((visibility("hidden")))
>> +uint64_t __llvm_profile_gather_value_data(
>> + __llvm_profile_value_data** ValueProfilingResults) {
>> +
>> + if (!ValueProfilingResults)
>> + return 0;
>> +
>> + if (0 == TotalValueDataCount)
>> + return 0;
>> +
>> + uint64_t NumDataCount = TotalValueDataCount;
>> + *ValueProfilingResults = (__llvm_profile_value_data*)
>> + calloc(NumDataCount, sizeof(__llvm_profile_value_data));
>> +
>> + if (!*ValueProfilingResults)
>> + return 0;
>> +
>> + uintptr_t CurrentFunctionIndex = -1;
>> + unsigned CurrentResultIndex = 0;
>> + const __llvm_profile_data *DataEnd = __llvm_profile_end_data();
>> + const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();
>> + for (const __llvm_profile_data *I = DataBegin; I != DataEnd; ++I) {
>> +
>> + CurrentFunctionIndex++;
>> +
>> + for (uint32_t VKI = VK_FIRST; VKI != VK_LAST; ++VKI) {
>> +
>> + if (!I->ValueCounters[VKI])
>> + continue;
>> +
>> + for (unsigned i = 0; i < I->NumValueSites[VKI]; ++i) {
>> +
>> + __llvm_profile_value_data *CurrentValueData =
>> I->ValueCounters[VKI][i];
>> +
>> + while (CurrentValueData) {
>> + (*ValueProfilingResults)[CurrentResultIndex] =
>> *CurrentValueData;
>> +
>> + // This assignment will only be read by llvm-profdata which
>> knows
>> + // that this value is no longer a pointer but an integer
>> index to
>> + // the __llvm_profile_data struct.
>> + // The casting below is there to suppress the warning.
>> + (*ValueProfilingResults)[CurrentResultIndex].Next =
>> + (__llvm_profile_value_data*)CurrentFunctionIndex;
>> +
>> + if (++CurrentResultIndex >= NumDataCount)
>> + return NumDataCount;
>
> This is an error condition, right? Should we return 0 here like we do
> for other errors? I'm worried about silently giving invalid data.
Not necessarily. ValueProfilingResults array is allocated statically at
the entrance to this function. In threaded environments, while the value
data is serialized and copied to the output array, another thread can
preempt and update the linked lists by adding an entry. This would change
the total number of the value data count. We do not want to write out of
bounds to the array allocated at the beginning of this routine.
>> + CurrentValueData = CurrentValueData->Next;
>> + }
>> + }
>> + }
>> + }
>> + return NumDataCount;
>> +}
>> +
>> +
>> Index: lib/profile/InstrProfiling.h
>> ===================================================================
>> --- lib/profile/InstrProfiling.h
>> +++ lib/profile/InstrProfiling.h
>> @@ -27,14 +27,35 @@
>>
>> #endif /* defined(__FreeBSD__) && defined(__i386__) */
>>
>> -#define PROFILE_HEADER_SIZE 7
>> +#define PROFILE_HEADER_SIZE 9
>> +
>> +typedef enum ValueKind {
>> + VK_FIRST = 0,
>> + VK_IndirectCallTarget = 0,
>> + VK_Reserved1 = 1,
>> + VK_Reserved2 = 2,
>> + VK_LAST = 3
>> +} __llvm_profile_value_kind;
>> +
>> +uint64_t TotalValueDataCount;
>
> This variable probably belongs in InstrProfiling.c.
Done.
>
>> +
>> +typedef struct __llvm_profile_value_data {
>> + uint64_t TargetValue;
>> + uint64_t NumTaken;
>> + uint32_t ValueKind;
>> + uint32_t CounterIndex;
>> + struct __llvm_profile_value_data *Next;
>> +} __llvm_profile_value_data;
>>
>> typedef struct __llvm_profile_data {
>> const uint32_t NameSize;
>> const uint32_t NumCounters;
>> const uint64_t FuncHash;
>> const char *const Name;
>> uint64_t *const Counters;
>> + const uint8_t *FunctionPointer;
>
> Is this used?
This is used during the reading of the profile data i.e. in
RawInstrProfReader. The above function pointer entries are compared to the
function addresses recorded through indirect call profiling
instrumentation.
>
>> + const uint32_t NumValueSites[VK_LAST];
>> + __llvm_profile_value_data **ValueCounters[VK_LAST];
>> } __llvm_profile_data;
>>
>> /*!
>> @@ -58,6 +79,23 @@
>> uint64_t *__llvm_profile_end_counters(void);
>>
>> /*!
>> + * \brief Counts the number of times a target value of ValueKind is
>> seen.
>> + *
>> + * Records the target value for a given ValueKind if not seen before.
>> Otherwise,
>> + * increments the counter associated w/ the target value.
>> + */
>> +void __llvm_profile_instrument_target(uint32_t ValueKind,
>> + uint64_t TargetValue, void *Data_, uint32_t CounterIndex);
>> +
>> +/*!
>> + * \brief Prepares the value profiling data for output.
>> + *
>> + * Prepares the value profiling data into a single array.
>> + */
>> +uint64_t __llvm_profile_gather_value_data(
>> + __llvm_profile_value_data** ValueProfilingResults);
>> +
>> +/*!
>> * \brief Write instrumentation data to the current file.
>> *
>> * Writes to the file with the last name given to \a
>> __llvm_profile_set_filename(),
>> Index: lib/profile/InstrProfilingBuffer.c
>> ===================================================================
>> --- lib/profile/InstrProfilingBuffer.c
>> +++ lib/profile/InstrProfilingBuffer.c
>> @@ -68,11 +68,14 @@
>> * Match logic in __llvm_profile_write_file().
>> */
>>
>> + __llvm_profile_value_data *ValueDataBegin = NULL;
>> +
>> /* Calculate size of sections. */
>> const uint64_t DataSize = DataEnd - DataBegin;
>> const uint64_t CountersSize = CountersEnd - CountersBegin;
>> const uint64_t NamesSize = NamesEnd - NamesBegin;
>> const uint64_t Padding = sizeof(uint64_t) - NamesSize %
>> sizeof(uint64_t);
>> + const uint64_t ValueDataSize = 0;
>
> I guess this is supposed to do:
>
> __llvm_profile_gather_value_data(&ValueDataBegin);
This is correct. However, on the buffer access API's the size of the
buffer is calculated so that the caller of the write API allocates the
necessary buffer. A pointer to the buffer is passed back to the write
function for the data to be dumped to this memory. In my implementation, I
kept the buffer API's unmodified - other than changing the format to match
that of version 2's. I'd like to know if a re-alloc on the passed in (char
*Buffer) be in order as a change to accommodate value profiling's dynamic
memory needs.
>
> ?
>
>>
>> /* Enough zeroes for padding. */
>> const char Zeroes[sizeof(uint64_t)] = {0};
>> @@ -86,6 +89,8 @@
>> Header[4] = NamesSize;
>> Header[5] = (uintptr_t)CountersBegin;
>> Header[6] = (uintptr_t)NamesBegin;
>> + Header[7] = ValueDataSize;
>> + Header[8] = (uintptr_t)ValueDataBegin;
>>
>> /* Write the data. */
>> #define UPDATE_memcpy(Data, Size) \
>> Index: lib/profile/InstrProfilingFile.c
>> ===================================================================
>> --- lib/profile/InstrProfilingFile.c
>> +++ lib/profile/InstrProfilingFile.c
>> @@ -23,12 +23,20 @@
>> const uint64_t *CountersEnd = __llvm_profile_end_counters();
>> const char *NamesBegin = __llvm_profile_begin_names();
>> const char *NamesEnd = __llvm_profile_end_names();
>> + __llvm_profile_value_data *ValueDataBegin = NULL;
>>
>> /* Calculate size of sections. */
>> const uint64_t DataSize = DataEnd - DataBegin;
>> const uint64_t CountersSize = CountersEnd - CountersBegin;
>> const uint64_t NamesSize = NamesEnd - NamesBegin;
>> const uint64_t Padding = sizeof(uint64_t) - NamesSize %
>> sizeof(uint64_t);
>> + const uint64_t ValueDataSize =
>> + __llvm_profile_gather_value_data(&ValueDataBegin);
>> +
>> + const uint64_t ValueDataSizeInBytes =
>> + ValueDataSize * sizeof(__llvm_profile_value_data);
>> + const uint64_t Padding2 = sizeof(uint64_t) -
>> + ValueDataSizeInBytes % sizeof(uint64_t);
>>
>> /* Enough zeroes for padding. */
>> const char Zeroes[sizeof(uint64_t)] = {0};
>> @@ -42,6 +50,8 @@
>> Header[4] = NamesSize;
>> Header[5] = (uintptr_t)CountersBegin;
>> Header[6] = (uintptr_t)NamesBegin;
>> + Header[7] = ValueDataSize;
>> + Header[8] = (uintptr_t)ValueDataBegin;
>>
>> /* Write the data. */
>> #define CHECK_fwrite(Data, Size, Length, File) \
>> @@ -51,8 +61,11 @@
>> CHECK_fwrite(CountersBegin, sizeof(uint64_t), CountersSize, File);
>> CHECK_fwrite(NamesBegin, sizeof(char), NamesSize, File);
>> CHECK_fwrite(Zeroes, sizeof(char), Padding, File);
>> + CHECK_fwrite(ValueDataBegin,
>> + sizeof(__llvm_profile_value_data), ValueDataSize, File);
>> + CHECK_fwrite(Zeroes, sizeof(char), Padding2, File);
>> #undef CHECK_fwrite
>> -
>> + free(ValueDataBegin);
>> return 0;
>> }
>>
>
More information about the llvm-commits
mailing list