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

[David Li via llvm-commits]

davidxl marked an inline comment as done.

================
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];
+
----------------
vsk wrote:
> 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.
The comment says the number of element is NumValueSites. Declaring the array with 1 element serves the purpose of indicating that there is at least one site profiled. Just added a comment that there is one element guaranteed.

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

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


http://reviews.llvm.org/D14401