[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 3 inline comments as done.
davidxl added a comment.

There is no need for binary blob testing -- the runtime and raw format change is still pending, so this is no need to keep the backward compatibility. Besides the patch basically removes the old format support, it will break with the test.


================
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];
+
----------------
uint8_t is an implementation detail (where the packing comes from). In practice, most of the value sites are cold with zero value tracked and large number of tracked values will be useless for compiler and is a waste of space, this is why the runtime will guarantee (change by Betul under review) the value count for each site is limited.

However I think the interface should still use uint32_t. Also note that the SiteCountArray is variable length, thus it is declared with element == 1.

================
Comment at: include/llvm/ProfileData/InstrProf.h:546
@@ -443,4 +545,3 @@
 // InstrProfilingBuffer.c.
-
 struct Header {
   const uint64_t Magic;
----------------
Yes -- there is a plan for that.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:25
@@ +24,3 @@
+uint32_t ValueProfRecord::getHeaderSize(uint32_t NumValueSites) {
+  size_t Size = sizeof(ValueProfRecord) + sizeof(uint8_t) * (NumValueSites - 1);
+  // Round the size to multiple of 8 bytes.
----------------
There was a bug there that may waste space. Changed to use offsetof.


http://reviews.llvm.org/D14401