[compiler-rt] r253500 - [PGO] Refactor File and Buffer API profile writing code
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 18 23:00:21 PST 2015
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.
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/20151118/253fc677/attachment.html>
More information about the llvm-commits
mailing list