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

[Vedant Kumar via llvm-commits]

vsk added a subscriber: vsk.
vsk added a comment.

Thanks! I have a few inline comments.

As I understand it, we need to maintain backwards compatibility for the indexed data format. Could you commit a binary blob which uses the old indexed format (in which value site counts are right next to value kinds), and show that we can still read it?


================
Comment at: include/llvm/ProfileData/InstrProf.h:415
@@ +414,3 @@
+  // The number of value profile sites. It is guaranteed to be non-zero;
+  // otherwise the recordfor this kind won't be emitted.
+  uint32_t NumValueSites;
----------------
recordfor -> record for

================
Comment at: include/llvm/ProfileData/InstrProf.h:419
@@ +418,3 @@
+  // values for each value site. The size of the array is NumValueSites.
+  uint8_t SiteCountArray[1];
+
----------------
`SiteCountArray` has length `NumValueSites`, but it's declared here with length 1. Your patch also stores `uint32_t` into this array, which is an array of `uint8_t`.

I was confused by this when I first read through your patch. Could you change this to `uint32_t SiteCountArray[]`? If you mean for this to be an array of `uint8_t`, please change the return type of `getNumValueDataForSite`.

================
Comment at: include/llvm/ProfileData/InstrProf.h:421
@@ +420,3 @@
+
+#if DO_NOT_DEFINE_ME
+  // The fake declaration is for documentation purpose only.
----------------
I think the convention is to use `#if 0` for this in llvm.

================
Comment at: include/llvm/ProfileData/InstrProf.h:469
@@ +468,3 @@
+
+#if DO_NOT_DEFINE_ME
+  // Following are a sequence of variable length records. The prefix/header
----------------
Same nit as above.

================
Comment at: include/llvm/ProfileData/InstrProf.h:543
@@ -440,4 +542,3 @@
 // InstrProfilingBuffer.c.
-
 struct Header {
   const uint64_t Magic;
----------------
Do you have any plans for pulling in this definition into compiler-rt, and removing the duplicate definition there?

================
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.
----------------
I think this line would have to change to `sizeof(ValueProfRecord) + sizeof(uint8_t) * NumValueSites` if you take my earlier suggestion. I think it would be cleaner this way.


http://reviews.llvm.org/D14401