[compiler-rt] r253500 - [PGO] Refactor File and Buffer API profile writing code
Sean Silva via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 18 22:36:48 PST 2015
Nice! This has been needed for a long time.
Some review comments:
- BufferWriter and FileWriter do not follow the LLVM naming convention for
functions
- `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.
- why does a declaration of llvmWriteProfDataImpl need to be in a header?
It should be a static helper, right?
- 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.
-- 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/04165b52/attachment-0001.html>
More information about the llvm-commits
mailing list