<div dir="ltr"><span style="font-size:13px">Nice! This has been needed for a long time.</span><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">Some review comments:</span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">- BufferWriter and </span><span style="font-size:13px">FileWriter </span><span style="font-size:13px">do not follow the LLVM naming convention for functions</span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">- `</span><span style="font-size:13px">void *BufferOrFile` is better named as `</span><span style="font-size:13px">void *Data` or `void *Closure` or similar, as we don't want to suggest that we assume a specific meaning. (`grep 'void \*'` in `</span>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.<span style="font-size:13px"><br></span></div><div><br></div><div>- why does a declaration of <span style="font-size:13px">llvmWriteProfDataImpl need to be in a header? It should be a static helper, right?</span></div><div><br></div><div>- taking `void **` in a callback and mutating through it is an unusual pattern. It would simplify things and make them more understandable if <span style="font-size:13px">WriterCallback is only called once, and is called with an array of "stringref" (</span>like the use of<span style="font-size:13px"> `struct iovec` in writev syscall interface).</span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">That is, I would suggest the following signatures:</span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">typedef struct __llvm_profile_iovec {</span></div><div><span style="font-size:13px"> uint8_t *Base;</span></div><div><span style="font-size:13px"> size_t Len;</span></div><div><span style="font-size:13px">} </span><span style="font-size:13px">__llvm_profile_iovec</span><span style="font-size:13px">;</span></div><div><span style="font-size:13px">typedef size_t (*WriterCallback)(const __llvm_profile_iovec *Iovecs, int NumIovecs,</span><span style="font-size:13px"> void *WriterCtx);</span><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">int llvmWriteProfData(const uint8_t *ValueDataBegin, const uint64_t ValueDataSize, WriterCallback Writer, void *WriterCtx);</span><span style="font-size:13px"><br></span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">(names for the iovec stuff is just an example; I have no particular attachment to those choices)</span></div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px">In </span><span style="font-size:13px">llvmWriteProfDataImpl we</span><span style="font-size:13px"> then create an array of </span><span style="font-size:13px">__llvm_profile_iovec to pass to WriterCallback.</span></div><div><span style="font-size:13px"><br></span></div><div>-- Sean Silva</div><div><span style="font-size:13px"><br></span></div><div><span style="font-size:13px"><br></span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 18, 2015 at 1:08 PM, Xinliang David Li via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: davidxl<br>
Date: Wed Nov 18 15:08:03 2015<br>
New Revision: 253500<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=253500&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=253500&view=rev</a><br>
Log:<br>
[PGO] Refactor File and Buffer API profile writing code<br>
<br>
With this change, Buffer API and File API implementations<br>
are unified.<br>
<br>
Differential Revision: <a href="http://reviews.llvm.org/D14692" rel="noreferrer" target="_blank">http://reviews.llvm.org/D14692</a><br>
<br>
<br>
Added:<br>
compiler-rt/trunk/lib/profile/InstrProfilingWriter.c<br>
Modified:<br>
compiler-rt/trunk/lib/profile/CMakeLists.txt<br>
compiler-rt/trunk/lib/profile/InstrProfilingBuffer.c<br>
compiler-rt/trunk/lib/profile/InstrProfilingFile.c<br>
compiler-rt/trunk/lib/profile/InstrProfilingInternal.h<br>
<br>
Modified: compiler-rt/trunk/lib/profile/CMakeLists.txt<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/CMakeLists.txt?rev=253500&r1=253499&r2=253500&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/CMakeLists.txt?rev=253500&r1=253499&r2=253500&view=diff</a><br>
==============================================================================<br>
--- compiler-rt/trunk/lib/profile/CMakeLists.txt (original)<br>
+++ compiler-rt/trunk/lib/profile/CMakeLists.txt Wed Nov 18 15:08:03 2015<br>
@@ -5,6 +5,7 @@ set(PROFILE_SOURCES<br>
InstrProfiling.c<br>
InstrProfilingBuffer.c<br>
InstrProfilingFile.c<br>
+ InstrProfilingWriter.c<br>
InstrProfilingPlatformDarwin.c<br>
InstrProfilingPlatformLinux.c<br>
InstrProfilingPlatformOther.c<br>
<br>
Modified: compiler-rt/trunk/lib/profile/InstrProfilingBuffer.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingBuffer.c?rev=253500&r1=253499&r2=253500&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingBuffer.c?rev=253500&r1=253499&r2=253500&view=diff</a><br>
==============================================================================<br>
--- compiler-rt/trunk/lib/profile/InstrProfilingBuffer.c (original)<br>
+++ compiler-rt/trunk/lib/profile/InstrProfilingBuffer.c Wed Nov 18 15:08:03 2015<br>
@@ -37,75 +37,28 @@ uint64_t __llvm_profile_get_size_for_buf<br>
const uint64_t NamesSize = PROFILE_RANGE_SIZE(Names) * sizeof(char);<br>
const uint8_t Padding = __llvm_profile_get_num_padding_bytes(NamesSize);<br>
return sizeof(__llvm_profile_header) +<br>
- PROFILE_RANGE_SIZE(Data) * sizeof(__llvm_profile_data) +<br>
- PROFILE_RANGE_SIZE(Counters) * sizeof(uint64_t) +<br>
- NamesSize + Padding;<br>
+ PROFILE_RANGE_SIZE(Data) * sizeof(__llvm_profile_data) +<br>
+ PROFILE_RANGE_SIZE(Counters) * sizeof(uint64_t) + NamesSize + Padding;<br>
}<br>
<br>
-__attribute__((visibility("hidden")))<br>
-int __llvm_profile_write_buffer(char *Buffer) {<br>
- /* Match logic in __llvm_profile_get_size_for_buffer().<br>
- * Match logic in __llvm_profile_write_file().<br>
- */<br>
- const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();<br>
- const __llvm_profile_data *DataEnd = __llvm_profile_end_data();<br>
- const uint64_t *CountersBegin = __llvm_profile_begin_counters();<br>
- const uint64_t *CountersEnd = __llvm_profile_end_counters();<br>
- const char *NamesBegin = __llvm_profile_begin_names();<br>
- const char *NamesEnd = __llvm_profile_end_names();<br>
+static size_t BufferWriter(const void *Data, size_t ElmSize, size_t NumElm,<br>
+ void **Buffer) {<br>
+ size_t Length = ElmSize * NumElm;<br>
+ memcpy(*Buffer, Data, Length);<br>
+ *(char **)Buffer += Length;<br>
+ return NumElm;<br>
+}<br>
<br>
- return __llvm_profile_write_buffer_internal(Buffer, DataBegin, DataEnd,<br>
- CountersBegin, CountersEnd,<br>
- NamesBegin, NamesEnd);<br>
+__attribute__((visibility("hidden"))) int<br>
+__llvm_profile_write_buffer(char *Buffer) {<br>
+ return llvmWriteProfData(Buffer, 0, 0, BufferWriter);<br>
}<br>
<br>
-__attribute__((visibility("hidden")))<br>
-int __llvm_profile_write_buffer_internal(<br>
+__attribute__((visibility("hidden"))) int __llvm_profile_write_buffer_internal(<br>
char *Buffer, const __llvm_profile_data *DataBegin,<br>
const __llvm_profile_data *DataEnd, const uint64_t *CountersBegin,<br>
const uint64_t *CountersEnd, const char *NamesBegin, const char *NamesEnd) {<br>
- /* Match logic in __llvm_profile_get_size_for_buffer().<br>
- * Match logic in __llvm_profile_write_file().<br>
- */<br>
-<br>
- /* Calculate size of sections. */<br>
- const uint64_t DataSize = DataEnd - DataBegin;<br>
- const uint64_t CountersSize = CountersEnd - CountersBegin;<br>
- const uint64_t NamesSize = NamesEnd - NamesBegin;<br>
- const uint8_t Padding = __llvm_profile_get_num_padding_bytes(NamesSize);<br>
-<br>
- /* Enough zeroes for padding. */<br>
- const char Zeroes[sizeof(uint64_t)] = {0};<br>
-<br>
- /* Create the header. */<br>
- __llvm_profile_header Header;<br>
-<br>
- if (!DataSize)<br>
- return 0;<br>
-<br>
- Header.Magic = __llvm_profile_get_magic();<br>
- Header.Version = __llvm_profile_get_version();<br>
- Header.DataSize = DataSize;<br>
- Header.CountersSize = CountersSize;<br>
- Header.NamesSize = NamesSize;<br>
- Header.CountersDelta = (uintptr_t)CountersBegin;<br>
- Header.NamesDelta = (uintptr_t)NamesBegin;<br>
- Header.ValueKindLast = VK_LAST;<br>
- Header.ValueDataSize = 0;<br>
- Header.ValueDataDelta = (uintptr_t)NULL;<br>
-<br>
- /* Write the data. */<br>
-#define UPDATE_memcpy(Data, Size) \<br>
- do { \<br>
- memcpy(Buffer, Data, Size); \<br>
- Buffer += Size; \<br>
- } while (0)<br>
- UPDATE_memcpy(&Header, sizeof(__llvm_profile_header));<br>
- UPDATE_memcpy(DataBegin, DataSize * sizeof(__llvm_profile_data));<br>
- UPDATE_memcpy(CountersBegin, CountersSize * sizeof(uint64_t));<br>
- UPDATE_memcpy(NamesBegin, NamesSize * sizeof(char));<br>
- UPDATE_memcpy(Zeroes, Padding * sizeof(char));<br>
-#undef UPDATE_memcpy<br>
-<br>
- return 0;<br>
+ return llvmWriteProfDataImpl(Buffer, BufferWriter, DataBegin, DataEnd,<br>
+ CountersBegin, CountersEnd, 0, 0, NamesBegin,<br>
+ NamesEnd);<br>
}<br>
<br>
Modified: compiler-rt/trunk/lib/profile/InstrProfilingFile.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingFile.c?rev=253500&r1=253499&r2=253500&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingFile.c?rev=253500&r1=253499&r2=253500&view=diff</a><br>
==============================================================================<br>
--- compiler-rt/trunk/lib/profile/InstrProfilingFile.c (original)<br>
+++ compiler-rt/trunk/lib/profile/InstrProfilingFile.c Wed Nov 18 15:08:03 2015<br>
@@ -9,6 +9,7 @@<br>
<br>
#include "InstrProfiling.h"<br>
#include "InstrProfilingUtil.h"<br>
+#include "InstrProfilingInternal.h"<br>
#include <errno.h><br>
#include <stdio.h><br>
#include <stdlib.h><br>
@@ -16,57 +17,18 @@<br>
<br>
#define UNCONST(ptr) ((void *)(uintptr_t)(ptr))<br>
<br>
-static int writeFile(FILE *File) {<br>
- /* Match logic in __llvm_profile_write_buffer(). */<br>
- const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();<br>
- const __llvm_profile_data *DataEnd = __llvm_profile_end_data();<br>
- const uint64_t *CountersBegin = __llvm_profile_begin_counters();<br>
- const uint64_t *CountersEnd = __llvm_profile_end_counters();<br>
- const char *NamesBegin = __llvm_profile_begin_names();<br>
- const char *NamesEnd = __llvm_profile_end_names();<br>
+static size_t FileWriter(const void *Data, size_t ElmSize, size_t NumElm,<br>
+ void **File) {<br>
+ return fwrite(Data, ElmSize, NumElm, (FILE *)*File);<br>
+}<br>
uint8_t *ValueDataBegin = NULL;<br>
<br>
- /* Calculate size of sections. */<br>
- const uint64_t DataSize = DataEnd - DataBegin;<br>
- const uint64_t CountersSize = CountersEnd - CountersBegin;<br>
- const uint64_t NamesSize = NamesEnd - NamesBegin;<br>
- const uint8_t Padding = __llvm_profile_get_num_padding_bytes(NamesSize);<br>
- const uint64_t ValueDataSize =<br>
- __llvm_profile_gather_value_data(&ValueDataBegin);<br>
-<br>
- /* Enough zeroes for padding. */<br>
- const char Zeroes[sizeof(uint64_t)] = {0};<br>
-<br>
- /* Create the header. */<br>
- __llvm_profile_header Header;<br>
-<br>
- if (!DataSize)<br>
- return 0;<br>
-<br>
- Header.Magic = __llvm_profile_get_magic();<br>
- Header.Version = __llvm_profile_get_version();<br>
- Header.DataSize = DataSize;<br>
- Header.CountersSize = CountersSize;<br>
- Header.NamesSize = NamesSize;<br>
- Header.CountersDelta = (uintptr_t)CountersBegin;<br>
- Header.NamesDelta = (uintptr_t)NamesBegin;<br>
- Header.ValueKindLast = VK_LAST;<br>
- Header.ValueDataSize = ValueDataSize;<br>
- Header.ValueDataDelta = (uintptr_t)ValueDataBegin;<br>
-<br>
- /* Write the data. */<br>
-#define CHECK_fwrite(Data, Size, Length, File) \<br>
- do { if (fwrite(Data, Size, Length, File) != Length) return -1; } while (0)<br>
- CHECK_fwrite(&Header, sizeof(__llvm_profile_header), 1, File);<br>
- CHECK_fwrite(DataBegin, sizeof(__llvm_profile_data), DataSize, File);<br>
- CHECK_fwrite(CountersBegin, sizeof(uint64_t), CountersSize, File);<br>
- CHECK_fwrite(NamesBegin, sizeof(char), NamesSize, File);<br>
- CHECK_fwrite(Zeroes, sizeof(char), Padding, File);<br>
- CHECK_fwrite(ValueDataBegin,<br>
- sizeof(__llvm_profile_value_data), ValueDataSize, File);<br>
-#undef CHECK_fwrite<br>
- free(ValueDataBegin);<br>
- return 0;<br>
+static int writeFile(FILE *File) {<br>
+ uint8_t *ValueDataBegin = NULL;<br>
+ const uint64_t ValueDataSize = __llvm_profile_gather_value_data(&ValueDataBegin);<br>
+ int r = llvmWriteProfData(File, ValueDataBegin, ValueDataSize, FileWriter);<br>
+ free (ValueDataBegin);<br>
+ return r;<br>
}<br>
<br>
static int writeFileWithName(const char *OutputName) {<br>
<br>
Modified: compiler-rt/trunk/lib/profile/InstrProfilingInternal.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingInternal.h?rev=253500&r1=253499&r2=253500&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingInternal.h?rev=253500&r1=253499&r2=253500&view=diff</a><br>
==============================================================================<br>
--- compiler-rt/trunk/lib/profile/InstrProfilingInternal.h (original)<br>
+++ compiler-rt/trunk/lib/profile/InstrProfilingInternal.h Wed Nov 18 15:08:03 2015<br>
@@ -11,6 +11,7 @@<br>
#define PROFILE_INSTRPROFILING_INTERNALH_<br>
<br>
#include "InstrProfiling.h"<br>
+#include "stddef.h"<br>
<br>
/*!<br>
* \brief Write instrumentation data to the given buffer, given explicit<br>
@@ -37,4 +38,18 @@ int __llvm_profile_write_buffer_internal<br>
const __llvm_profile_data *DataEnd, const uint64_t *CountersBegin,<br>
const uint64_t *CountersEnd, const char *NamesBegin, const char *NamesEnd);<br>
<br>
+/*!<br>
+ * This is an internal function not intended to be used externally.<br>
+ */<br>
+typedef size_t (*WriterCallback)(const void *Data, size_t ElmS, size_t NumElm,<br>
+ void **BufferOrFile);<br>
+int llvmWriteProfData(void *BufferOrFile, const uint8_t *ValueDataBegin, const uint64_t ValueDataSize, WriterCallback Writer);<br>
+int llvmWriteProfDataImpl(void *BufferOrFile, WriterCallback Writer,<br>
+ const __llvm_profile_data *DataBegin,<br>
+ const __llvm_profile_data *DataEnd,<br>
+ const uint64_t *CountersBegin, const uint64_t *CountersEnd,<br>
+ const uint8_t *ValueDataBegin, const uint64_t ValueDataSize,<br>
+ const char *NamesBegin,<br>
+ const char *NamesEnd);<br>
+<br>
#endif<br>
<br>
Added: compiler-rt/trunk/lib/profile/InstrProfilingWriter.c<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingWriter.c?rev=253500&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/profile/InstrProfilingWriter.c?rev=253500&view=auto</a><br>
==============================================================================<br>
--- compiler-rt/trunk/lib/profile/InstrProfilingWriter.c (added)<br>
+++ compiler-rt/trunk/lib/profile/InstrProfilingWriter.c Wed Nov 18 15:08:03 2015<br>
@@ -0,0 +1,77 @@<br>
+/*===- InstrProfilingWriter.c - Write instrumentation to a file or buffer -===*\<br>
+|*<br>
+|* The LLVM Compiler Infrastructure<br>
+|*<br>
+|* This file is distributed under the University of Illinois Open Source<br>
+|* License. See LICENSE.TXT for details.<br>
+|*<br>
+\*===----------------------------------------------------------------------===*/<br>
+<br>
+#include "InstrProfiling.h"<br>
+#include "InstrProfilingInternal.h"<br>
+<br>
+__attribute__((visibility("hidden"))) int<br>
+llvmWriteProfData(void *BufferOrFile, const uint8_t * ValueDataBegin, const uint64_t ValueDataSize, WriterCallback Writer) {<br>
+ /* Match logic in __llvm_profile_write_buffer(). */<br>
+ const __llvm_profile_data *DataBegin = __llvm_profile_begin_data();<br>
+ const __llvm_profile_data *DataEnd = __llvm_profile_end_data();<br>
+ const uint64_t *CountersBegin = __llvm_profile_begin_counters();<br>
+ const uint64_t *CountersEnd = __llvm_profile_end_counters();<br>
+ const char *NamesBegin = __llvm_profile_begin_names();<br>
+ const char *NamesEnd = __llvm_profile_end_names();<br>
+ return llvmWriteProfDataImpl(BufferOrFile, Writer, DataBegin, DataEnd,<br>
+ CountersBegin, CountersEnd,<br>
+ ValueDataBegin, ValueDataSize,<br>
+ NamesBegin,<br>
+ NamesEnd);<br>
+}<br>
+<br>
+__attribute__((visibility("hidden"))) int llvmWriteProfDataImpl(<br>
+ void *BufferOrFile, WriterCallback Writer,<br>
+ const __llvm_profile_data *DataBegin, const __llvm_profile_data *DataEnd,<br>
+ const uint64_t *CountersBegin, const uint64_t *CountersEnd,<br>
+ const uint8_t *ValueDataBegin, const uint64_t ValueDataSize,<br>
+ const char *NamesBegin, const char *NamesEnd) {<br>
+<br>
+ /* Calculate size of sections. */<br>
+ const uint64_t DataSize = DataEnd - DataBegin;<br>
+ const uint64_t CountersSize = CountersEnd - CountersBegin;<br>
+ const uint64_t NamesSize = NamesEnd - NamesBegin;<br>
+ const uint64_t Padding = __llvm_profile_get_num_padding_bytes(NamesSize);<br>
+<br>
+ /* Enough zeroes for padding. */<br>
+ const char Zeroes[sizeof(uint64_t)] = {0};<br>
+<br>
+ /* Create the header. */<br>
+ __llvm_profile_header Header;<br>
+<br>
+ if (!DataSize)<br>
+ return 0;<br>
+<br>
+ Header.Magic = __llvm_profile_get_magic();<br>
+ Header.Version = __llvm_profile_get_version();<br>
+ Header.DataSize = DataSize;<br>
+ Header.CountersSize = CountersSize;<br>
+ Header.NamesSize = NamesSize;<br>
+ Header.CountersDelta = (uintptr_t)CountersBegin;<br>
+ Header.NamesDelta = (uintptr_t)NamesBegin;<br>
+ Header.ValueKindLast = VK_LAST;<br>
+ Header.ValueDataSize = ValueDataSize;<br>
+ Header.ValueDataDelta = (uintptr_t)ValueDataBegin;<br>
+<br>
+/* Write the data. */<br>
+#define CHECK_write(Data, Size, Length, BuffOrFile) \<br>
+ do { \<br>
+ if (Writer(Data, Size, Length, &BuffOrFile) != Length) \<br>
+ return -1; \<br>
+ } while (0)<br>
+ CHECK_write(&Header, sizeof(__llvm_profile_header), 1, BufferOrFile);<br>
+ CHECK_write(DataBegin, sizeof(__llvm_profile_data), DataSize, BufferOrFile);<br>
+ CHECK_write(CountersBegin, sizeof(uint64_t), CountersSize, BufferOrFile);<br>
+ CHECK_write(NamesBegin, sizeof(char), NamesSize, BufferOrFile);<br>
+ CHECK_write(Zeroes, sizeof(char), Padding, BufferOrFile);<br>
+ if (ValueDataBegin)<br>
+ CHECK_write(ValueDataBegin, sizeof(char), ValueDataSize, BufferOrFile);<br>
+#undef CHECK_write<br>
+ return 0;<br>
+}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>