[compiler-rt] r253500 - [PGO] Refactor File and Buffer API profile writing code
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 19 19:15:17 PST 2015
The only reason the void** hack is used is because the buffer writer needs
to update the buffer pointer. I guess I can get rid of that (which I also
don't like either actually).
David
On Thu, Nov 19, 2015 at 7:00 PM, Sean Silva <chisophugis at gmail.com> wrote:
>
>
> 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/aee8819e/attachment.html>
More information about the llvm-commits
mailing list