[PATCH] D13843: Use ProfileData/InstrProfData.inc template file to define PGO runtime types

[Vedant Kumar via llvm-commits]

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

Thanks for the cleanup. Some inline comments --


================
Comment at: include/llvm/ProfileData/InstrProf.h:434
@@ -433,6 +433,3 @@
 struct ProfileData {
-  const uint32_t NameSize;
-  const uint32_t NumCounters;
-  const uint64_t FuncHash;
-  const IntPtrT NamePtr;
-  const IntPtrT CounterPtr;
+  #define INSTR_PROF_DATA(T, LLVMT, N, I) T N;
+  #include "llvm/ProfileData/InstrProfData.inc"
----------------
In 'InstrProfData.inc', the macro parameters names are a bit more descriptive. Could you use the same names here?

================
Comment at: include/llvm/ProfileData/InstrProf.h:458
@@ +457,3 @@
+struct __attribute__((packed)) CovMapFunctionRecord {
+  #define COVMAP_FUNC_RECORD(T, LLVMT, N, I) T N;
+  #include "llvm/ProfileData/InstrProfData.inc"
----------------
Same nit as above.

================
Comment at: lib/ProfileData/CoverageMappingReader.cpp:369
@@ -368,2 +368,3 @@
 
-    while (FunBuf < FunEnd) {
+    const coverage::CovMapFunctionRecord<T> *CFR =
+        reinterpret_cast<const coverage::CovMapFunctionRecord<T> *>(FunBuf);
----------------
Could you make the l.h.s `auto *CFR`?

================
Comment at: lib/ProfileData/CoverageMappingReader.cpp:370
@@ +369,3 @@
+    const coverage::CovMapFunctionRecord<T> *CFR =
+        reinterpret_cast<const coverage::CovMapFunctionRecord<T> *>(FunBuf);
+    while ((const char *)CFR < FunEnd) {
----------------
I notice that the old code used unaligned accesses when unpacking `FunBuf`.

I think having an assertion here for the assumed alignment would make for good future-proofing. Could we assert that CFR->FuncHash is 8-byte aligned?



http://reviews.llvm.org/D13843