[PATCH] D14692: [PGO] Refactor common profile dumping code for File and Buffer API

[Vedant Kumar via llvm-commits]

vsk added a comment.

Thanks, just a few nits --


================
Comment at: lib/profile/InstrProfilingBuffer.c:44
@@ -44,12 +43,3 @@
 
-__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 ElmS, size_t NumElm,
+                           void **Buffer) {
----------------
`ElmS` -> `ElmSize`?

================
Comment at: lib/profile/InstrProfilingBuffer.c:44
@@ -44,12 +43,3 @@
 
-__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 ElmS, size_t NumElm,
+                           void **Buffer) {
----------------
vsk wrote:
> `ElmS` -> `ElmSize`?
C code in compiler-rt looks like it uses all lowercase and "_" separators in function names. We should preserve that style.

================
Comment at: lib/profile/InstrProfilingFile.c:20
@@ -18,44 +19,3 @@
 
-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();
-
-  /* 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 = sizeof(uint64_t) - NamesSize % sizeof(uint64_t);
-
-  /* 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;
-
-  /* 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);
-#undef CHECK_fwrite
-
-  return 0;
+static size_t FileWriter(const void *Data, size_t ElmS, size_t NumElm,
+                         void **File) {
----------------
Same `ElmS` nit

================
Comment at: lib/profile/InstrProfilingWriter.c:57
@@ +56,3 @@
+/* Write the data. */
+#define CHECK_fwrite(Data, Size, Length, BuffOrFile)                           \
+  do {                                                                         \
----------------
`CHECK_fwrite` -> `CHECK_write`


http://reviews.llvm.org/D14692