[PATCH] D14401: [PGO] Make indexed value profile data more compact and add structural definitions for the data format

Vedant Kumar via llvm-commits llvm-commits at lists.llvm.org
Fri Nov 6 13:09:23 PST 2015


vsk added a comment.

IIUC, when the raw format and runtime changes land, we will need a backwards compatibility test for the indexed format?

A few more inline comments --


================
Comment at: include/llvm/ProfileData/InstrProf.h:423
@@ +422,3 @@
+  // values for each value site. The size of the array is NumValueSites.
+  uint8_t SiteCountArray[1];
+
----------------
Ah ok.

When Betul's patch lands, we should really consider being consistent about using `uint8_t` for site counts everywhere, as it's a bit more accurate.

I see that `SiteCountArray` is variable length, but that's not obvious from a superficial reading of the code. It's declared as an array of length 1, which is surprising.

Declaring it as `[]` conveys the meaning more directly. Alternatively, you could define a method `getSiteCountArray() { return ((uint8_t *)this) + sizeof(ValueProfRecord); }`, and move the `SiteCountArray[]` declaration into the `#if 0` block.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:121
@@ +120,3 @@
+  using namespace support;
+  if (D + sizeof(uint64_t) > BufferEnd)
+    return instrprof_error::truncated;
----------------
The use of `sizeof(uint64_t)` in this function isn't self-explanatory. Afaict `sizeof(ValueProfData)` would work at least as well, and is a lot clearer.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:126
@@ +125,3 @@
+    uint64_t RawHeader;
+  };
+  RawHeader = endian::byte_swap<uint64_t, little>(*D);
----------------
Can you remove this union, or lift it into a more descriptive comment?

================
Comment at: lib/ProfileData/InstrProfReader.cpp:133
@@ +132,3 @@
+    return instrprof_error::malformed;
+  if (TotalSize & (sizeof(uint64_t) - 1))
+    return instrprof_error::malformed;
----------------
This would be much easier to understand as `if (TotalSize % sizeof(ValueProfData))`, which gets optimized properly later.


http://reviews.llvm.org/D14401





More information about the llvm-commits mailing list