[compiler-rt] r253500 - [PGO] Refactor File and Buffer API profile writing code
Sean Silva via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 19 19:00:39 PST 2015
On Wed, Nov 18, 2015 at 11:00 PM, Xinliang David Li <davidxl at google.com>
wrote:
>
>
> On Wed, Nov 18, 2015 at 10:36 PM, Sean Silva <chisophugis at gmail.com>
> wrote:
>
>> Nice! This has been needed for a long time.
>>
>> Some review comments:
>>
>> - BufferWriter and FileWriter do not follow the LLVM naming convention
>> for functions
>>
>
> Ok.
>
>
>> - `void *BufferOrFile` is better named as `void *Data` or `void
>> *Closure` or similar, as we don't want to suggest that we assume a specific
>> meaning. (`grep 'void \*'` in `llvm/include/llvm-c` suggests using `void
>> *Opaque` or `void *WriterCtx`). Also, the `void *` is usually taken as the
>> parameter after the callback function pointer itself.
>>
>
> ok.
>
>
>>
>> - why does a declaration of llvmWriteProfDataImpl need to be in a
>> header? It should be a static helper, right?
>>
>
> I explained it in the review comments. They are indeed internal functions
> (as can be seen from the naming scheme) -- they need to be in header
> because both InstrProfilingBuffer.c and InstrProfilingFile.c need access to
> them -- we can not merge InstrProfingBuffer.c and InstrProfilingFile.c
> because the latter depends on libc which buffer API need to avoid link in.
>
>
>
>> - taking `void **` in a callback and mutating through it is an unusual
>> pattern. It would simplify things and make them more understandable if WriterCallback
>> is only called once, and is called with an array of "stringref" (like
>> the use of `struct iovec` in writev syscall interface).
>>
>> That is, I would suggest the following signatures:
>>
>> typedef struct __llvm_profile_iovec {
>> uint8_t *Base;
>> size_t Len;
>> } __llvm_profile_iovec;
>> typedef size_t (*WriterCallback)(const __llvm_profile_iovec *Iovecs, int
>> NumIovecs, void *WriterCtx);
>> int llvmWriteProfData(const uint8_t *ValueDataBegin, const uint64_t
>> ValueDataSize, WriterCallback Writer, void *WriterCtx);
>>
>> (names for the iovec stuff is just an example; I have no particular
>> attachment to those choices)
>>
>> In llvmWriteProfDataImpl we then create an array of __llvm_profile_iovec
>> to pass to WriterCallback.
>>
>
>
> Writer callback's signature mimics fwrite's signature and it is pretty
> easy to read. I prefer keeping the current way until we find the need to
> extend the callback for more elaborate use cases.
>
Mutating the `void*` closure through a `void**` is really unusual and
confusing. I *strongly* prefer that we avoid that and stick to
tried-and-true, familiar patterns.
I feel less strongly about the fwrite-like interface (i.e. passing "number
of items" and "size of one item"), but it is just redundant and introduces
unnecessary complexity, since we are just writing byte buffers.
-- Sean Silva
>
> I will update the patch with other comments of yours.
>
> David
>
>
>
>>
>> -- Sean Silva
>>
>>
>>
>> On Wed, Nov 18, 2015 at 1:08 PM, Xinliang David Li via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: davidxl
>>> Date: Wed Nov 18 15:08:03 2015
>>> New Revision: 253500
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=253500&view=rev
>>> Log:
>>> [PGO] Refactor File and Buffer API profile writing code
>>>
>>> With this change, Buffer API and File API implementations
>>> are unified.
>>>
>>> Differential Revision: http://reviews.llvm.org/D14692
>>>
>>>
>>> Added:
>>> compiler-rt/trunk/lib/profile/InstrProfilingWriter.c
>>> Modified:
>>> compiler-rt/trunk/lib/profile/CMakeLists.txt
>>> compiler-rt/trunk/lib/profile/InstrProfilingBuffer.c
>>> compiler-rt/trunk/lib/profile/InstrProfilingFile.c
>>> compiler-rt/trunk/lib/profile/InstrProfilingInternal.h
>>>
>>> Modified: compiler-rt/trunk/lib/profile/CMakeLists.txt
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/CMakeLists.txt?rev=253500&r1=253499&r2=253500&view=diff
>>>
>>> ==============================================================================
>>> --- compiler-rt/trunk/lib/profile/CMakeLists.txt (original)
>>> +++ compiler-rt/trunk/lib/profile/CMakeLists.txt Wed Nov 18 15:08:03 2015
>>> @@ -5,6 +5,7 @@ set(PROFILE_SOURCES
>>> InstrProfiling.c
>>> InstrProfilingBuffer.c
>>> InstrProfilingFile.c
>>> + InstrProfilingWriter.c
>>> InstrProfilingPlatformDarwin.c
>>> InstrProfilingPlatformLinux.c
>>> InstrProfilingPlatformOther.c
>>>
>>> Modified: compiler-rt/trunk/lib/profile/InstrProfilingBuffer.c
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingBuffer.c?rev=253500&r1=253499&r2=253500&view=diff
>>>
>>> ==============================================================================
>>> --- compiler-rt/trunk/lib/profile/InstrProfilingBuffer.c (original)
>>> +++ compiler-rt/trunk/lib/profile/InstrProfilingBuffer.c Wed Nov 18
>>> 15:08:03 2015
>>> @@ -37,75 +37,28 @@ uint64_t __llvm_profile_get_size_for_buf
>>> const uint64_t NamesSize = PROFILE_RANGE_SIZE(Names) * sizeof(char);
>>> const uint8_t Padding =
>>> __llvm_profile_get_num_padding_bytes(NamesSize);
>>> return sizeof(__llvm_profile_header) +
>>> - PROFILE_RANGE_SIZE(Data) * sizeof(__llvm_profile_data) +
>>> - PROFILE_RANGE_SIZE(Counters) * sizeof(uint64_t) +
>>> - NamesSize + Padding;
>>> + PROFILE_RANGE_SIZE(Data) * sizeof(__llvm_profile_data) +
>>> + PROFILE_RANGE_SIZE(Counters) * sizeof(uint64_t) + NamesSize +
>>> Padding;
>>> }
>>>
>>> -__attribute__((visibility("hidden")))
>>> -int __llvm_profile_write_buffer(char *Buffer) {
>>> - /* Match logic in __llvm_profile_get_size_for_buffer().
>>> - * Match logic in __llvm_profile_write_file().
>>> - */
>>> - const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();
>>> - const __llvm_profile_data *DataEnd = __llvm_profile_end_data();
>>> - const uint64_t *CountersBegin = __llvm_profile_begin_counters();
>>> - const uint64_t *CountersEnd = __llvm_profile_end_counters();
>>> - const char *NamesBegin = __llvm_profile_begin_names();
>>> - const char *NamesEnd = __llvm_profile_end_names();
>>> +static size_t BufferWriter(const void *Data, size_t ElmSize, size_t
>>> NumElm,
>>> + void **Buffer) {
>>> + size_t Length = ElmSize * NumElm;
>>> + memcpy(*Buffer, Data, Length);
>>> + *(char **)Buffer += Length;
>>> + return NumElm;
>>> +}
>>>
>>> - return __llvm_profile_write_buffer_internal(Buffer, DataBegin,
>>> DataEnd,
>>> - CountersBegin,
>>> CountersEnd,
>>> - NamesBegin, NamesEnd);
>>> +__attribute__((visibility("hidden"))) int
>>> +__llvm_profile_write_buffer(char *Buffer) {
>>> + return llvmWriteProfData(Buffer, 0, 0, BufferWriter);
>>> }
>>>
>>> -__attribute__((visibility("hidden")))
>>> -int __llvm_profile_write_buffer_internal(
>>> +__attribute__((visibility("hidden"))) int
>>> __llvm_profile_write_buffer_internal(
>>> char *Buffer, const __llvm_profile_data *DataBegin,
>>> const __llvm_profile_data *DataEnd, const uint64_t *CountersBegin,
>>> const uint64_t *CountersEnd, const char *NamesBegin, const char
>>> *NamesEnd) {
>>> - /* Match logic in __llvm_profile_get_size_for_buffer().
>>> - * Match logic in __llvm_profile_write_file().
>>> - */
>>> -
>>> - /* Calculate size of sections. */
>>> - const uint64_t DataSize = DataEnd - DataBegin;
>>> - const uint64_t CountersSize = CountersEnd - CountersBegin;
>>> - const uint64_t NamesSize = NamesEnd - NamesBegin;
>>> - const uint8_t Padding =
>>> __llvm_profile_get_num_padding_bytes(NamesSize);
>>> -
>>> - /* Enough zeroes for padding. */
>>> - const char Zeroes[sizeof(uint64_t)] = {0};
>>> -
>>> - /* Create the header. */
>>> - __llvm_profile_header Header;
>>> -
>>> - if (!DataSize)
>>> - return 0;
>>> -
>>> - Header.Magic = __llvm_profile_get_magic();
>>> - Header.Version = __llvm_profile_get_version();
>>> - Header.DataSize = DataSize;
>>> - Header.CountersSize = CountersSize;
>>> - Header.NamesSize = NamesSize;
>>> - Header.CountersDelta = (uintptr_t)CountersBegin;
>>> - Header.NamesDelta = (uintptr_t)NamesBegin;
>>> - Header.ValueKindLast = VK_LAST;
>>> - Header.ValueDataSize = 0;
>>> - Header.ValueDataDelta = (uintptr_t)NULL;
>>> -
>>> - /* Write the data. */
>>> -#define UPDATE_memcpy(Data, Size) \
>>> - do { \
>>> - memcpy(Buffer, Data, Size); \
>>> - Buffer += Size; \
>>> - } while (0)
>>> - UPDATE_memcpy(&Header, sizeof(__llvm_profile_header));
>>> - UPDATE_memcpy(DataBegin, DataSize *
>>> sizeof(__llvm_profile_data));
>>> - UPDATE_memcpy(CountersBegin, CountersSize * sizeof(uint64_t));
>>> - UPDATE_memcpy(NamesBegin, NamesSize * sizeof(char));
>>> - UPDATE_memcpy(Zeroes, Padding * sizeof(char));
>>> -#undef UPDATE_memcpy
>>> -
>>> - return 0;
>>> + return llvmWriteProfDataImpl(Buffer, BufferWriter, DataBegin, DataEnd,
>>> + CountersBegin, CountersEnd, 0, 0,
>>> NamesBegin,
>>> + NamesEnd);
>>> }
>>>
>>> Modified: compiler-rt/trunk/lib/profile/InstrProfilingFile.c
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingFile.c?rev=253500&r1=253499&r2=253500&view=diff
>>>
>>> ==============================================================================
>>> --- compiler-rt/trunk/lib/profile/InstrProfilingFile.c (original)
>>> +++ compiler-rt/trunk/lib/profile/InstrProfilingFile.c Wed Nov 18
>>> 15:08:03 2015
>>> @@ -9,6 +9,7 @@
>>>
>>> #include "InstrProfiling.h"
>>> #include "InstrProfilingUtil.h"
>>> +#include "InstrProfilingInternal.h"
>>> #include <errno.h>
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> @@ -16,57 +17,18 @@
>>>
>>> #define UNCONST(ptr) ((void *)(uintptr_t)(ptr))
>>>
>>> -static int writeFile(FILE *File) {
>>> - /* Match logic in __llvm_profile_write_buffer(). */
>>> - const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();
>>> - const __llvm_profile_data *DataEnd = __llvm_profile_end_data();
>>> - const uint64_t *CountersBegin = __llvm_profile_begin_counters();
>>> - const uint64_t *CountersEnd = __llvm_profile_end_counters();
>>> - const char *NamesBegin = __llvm_profile_begin_names();
>>> - const char *NamesEnd = __llvm_profile_end_names();
>>> +static size_t FileWriter(const void *Data, size_t ElmSize, size_t
>>> NumElm,
>>> + void **File) {
>>> + return fwrite(Data, ElmSize, NumElm, (FILE *)*File);
>>> +}
>>> uint8_t *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 uint8_t Padding =
>>> __llvm_profile_get_num_padding_bytes(NamesSize);
>>> - const uint64_t ValueDataSize =
>>> - __llvm_profile_gather_value_data(&ValueDataBegin);
>>> -
>>> - /* Enough zeroes for padding. */
>>> - const char Zeroes[sizeof(uint64_t)] = {0};
>>> -
>>> - /* Create the header. */
>>> - __llvm_profile_header Header;
>>> -
>>> - if (!DataSize)
>>> - return 0;
>>> -
>>> - Header.Magic = __llvm_profile_get_magic();
>>> - Header.Version = __llvm_profile_get_version();
>>> - Header.DataSize = DataSize;
>>> - Header.CountersSize = CountersSize;
>>> - Header.NamesSize = NamesSize;
>>> - Header.CountersDelta = (uintptr_t)CountersBegin;
>>> - Header.NamesDelta = (uintptr_t)NamesBegin;
>>> - Header.ValueKindLast = VK_LAST;
>>> - Header.ValueDataSize = ValueDataSize;
>>> - Header.ValueDataDelta = (uintptr_t)ValueDataBegin;
>>> -
>>> - /* Write the data. */
>>> -#define CHECK_fwrite(Data, Size, Length, File) \
>>> - do { if (fwrite(Data, Size, Length, File) != Length) return -1; }
>>> while (0)
>>> - CHECK_fwrite(&Header, sizeof(__llvm_profile_header), 1, File);
>>> - CHECK_fwrite(DataBegin, sizeof(__llvm_profile_data), DataSize,
>>> File);
>>> - 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);
>>> -#undef CHECK_fwrite
>>> - free(ValueDataBegin);
>>> - return 0;
>>> +static int writeFile(FILE *File) {
>>> + uint8_t *ValueDataBegin = NULL;
>>> + const uint64_t ValueDataSize =
>>> __llvm_profile_gather_value_data(&ValueDataBegin);
>>> + int r = llvmWriteProfData(File, ValueDataBegin, ValueDataSize,
>>> FileWriter);
>>> + free (ValueDataBegin);
>>> + return r;
>>> }
>>>
>>> static int writeFileWithName(const char *OutputName) {
>>>
>>> Modified: compiler-rt/trunk/lib/profile/InstrProfilingInternal.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingInternal.h?rev=253500&r1=253499&r2=253500&view=diff
>>>
>>> ==============================================================================
>>> --- compiler-rt/trunk/lib/profile/InstrProfilingInternal.h (original)
>>> +++ compiler-rt/trunk/lib/profile/InstrProfilingInternal.h Wed Nov 18
>>> 15:08:03 2015
>>> @@ -11,6 +11,7 @@
>>> #define PROFILE_INSTRPROFILING_INTERNALH_
>>>
>>> #include "InstrProfiling.h"
>>> +#include "stddef.h"
>>>
>>> /*!
>>> * \brief Write instrumentation data to the given buffer, given explicit
>>> @@ -37,4 +38,18 @@ int __llvm_profile_write_buffer_internal
>>> const __llvm_profile_data *DataEnd, const uint64_t *CountersBegin,
>>> const uint64_t *CountersEnd, const char *NamesBegin, const char
>>> *NamesEnd);
>>>
>>> +/*!
>>> + * This is an internal function not intended to be used externally.
>>> + */
>>> +typedef size_t (*WriterCallback)(const void *Data, size_t ElmS, size_t
>>> NumElm,
>>> + void **BufferOrFile);
>>> +int llvmWriteProfData(void *BufferOrFile, const uint8_t
>>> *ValueDataBegin, const uint64_t ValueDataSize, WriterCallback Writer);
>>> +int llvmWriteProfDataImpl(void *BufferOrFile, WriterCallback Writer,
>>> + const __llvm_profile_data *DataBegin,
>>> + const __llvm_profile_data *DataEnd,
>>> + const uint64_t *CountersBegin, const uint64_t
>>> *CountersEnd,
>>> + const uint8_t *ValueDataBegin, const uint64_t
>>> ValueDataSize,
>>> + const char *NamesBegin,
>>> + const char *NamesEnd);
>>> +
>>> #endif
>>>
>>> Added: compiler-rt/trunk/lib/profile/InstrProfilingWriter.c
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingWriter.c?rev=253500&view=auto
>>>
>>> ==============================================================================
>>> --- compiler-rt/trunk/lib/profile/InstrProfilingWriter.c (added)
>>> +++ compiler-rt/trunk/lib/profile/InstrProfilingWriter.c Wed Nov 18
>>> 15:08:03 2015
>>> @@ -0,0 +1,77 @@
>>> +/*===- InstrProfilingWriter.c - Write instrumentation to a file or
>>> buffer -===*\
>>> +|*
>>> +|* The LLVM Compiler Infrastructure
>>> +|*
>>> +|* This file is distributed under the University of Illinois Open Source
>>> +|* License. See LICENSE.TXT for details.
>>> +|*
>>>
>>> +\*===----------------------------------------------------------------------===*/
>>> +
>>> +#include "InstrProfiling.h"
>>> +#include "InstrProfilingInternal.h"
>>> +
>>> +__attribute__((visibility("hidden"))) int
>>> +llvmWriteProfData(void *BufferOrFile, const uint8_t * ValueDataBegin,
>>> const uint64_t ValueDataSize, WriterCallback Writer) {
>>> + /* Match logic in __llvm_profile_write_buffer(). */
>>> + const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();
>>> + const __llvm_profile_data *DataEnd = __llvm_profile_end_data();
>>> + const uint64_t *CountersBegin = __llvm_profile_begin_counters();
>>> + const uint64_t *CountersEnd = __llvm_profile_end_counters();
>>> + const char *NamesBegin = __llvm_profile_begin_names();
>>> + const char *NamesEnd = __llvm_profile_end_names();
>>> + return llvmWriteProfDataImpl(BufferOrFile, Writer, DataBegin, DataEnd,
>>> + CountersBegin, CountersEnd,
>>> + ValueDataBegin, ValueDataSize,
>>> + NamesBegin,
>>> + NamesEnd);
>>> +}
>>> +
>>> +__attribute__((visibility("hidden"))) int llvmWriteProfDataImpl(
>>> + void *BufferOrFile, WriterCallback Writer,
>>> + const __llvm_profile_data *DataBegin, const __llvm_profile_data
>>> *DataEnd,
>>> + const uint64_t *CountersBegin, const uint64_t *CountersEnd,
>>> + const uint8_t *ValueDataBegin, const uint64_t ValueDataSize,
>>> + const char *NamesBegin, const char *NamesEnd) {
>>> +
>>> + /* 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 =
>>> __llvm_profile_get_num_padding_bytes(NamesSize);
>>> +
>>> + /* Enough zeroes for padding. */
>>> + const char Zeroes[sizeof(uint64_t)] = {0};
>>> +
>>> + /* Create the header. */
>>> + __llvm_profile_header Header;
>>> +
>>> + if (!DataSize)
>>> + return 0;
>>> +
>>> + Header.Magic = __llvm_profile_get_magic();
>>> + Header.Version = __llvm_profile_get_version();
>>> + Header.DataSize = DataSize;
>>> + Header.CountersSize = CountersSize;
>>> + Header.NamesSize = NamesSize;
>>> + Header.CountersDelta = (uintptr_t)CountersBegin;
>>> + Header.NamesDelta = (uintptr_t)NamesBegin;
>>> + Header.ValueKindLast = VK_LAST;
>>> + Header.ValueDataSize = ValueDataSize;
>>> + Header.ValueDataDelta = (uintptr_t)ValueDataBegin;
>>> +
>>> +/* Write the data. */
>>> +#define CHECK_write(Data, Size, Length, BuffOrFile)
>>> \
>>> + do {
>>> \
>>> + if (Writer(Data, Size, Length, &BuffOrFile) != Length)
>>> \
>>> + return -1;
>>> \
>>> + } while (0)
>>> + CHECK_write(&Header, sizeof(__llvm_profile_header), 1, BufferOrFile);
>>> + CHECK_write(DataBegin, sizeof(__llvm_profile_data), DataSize,
>>> BufferOrFile);
>>> + CHECK_write(CountersBegin, sizeof(uint64_t), CountersSize,
>>> BufferOrFile);
>>> + CHECK_write(NamesBegin, sizeof(char), NamesSize, BufferOrFile);
>>> + CHECK_write(Zeroes, sizeof(char), Padding, BufferOrFile);
>>> + if (ValueDataBegin)
>>> + CHECK_write(ValueDataBegin, sizeof(char), ValueDataSize,
>>> BufferOrFile);
>>> +#undef CHECK_write
>>> + return 0;
>>> +}
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151119/7a50fe19/attachment-0001.html>
More information about the llvm-commits
mailing list