[PATCH] D13308: Value profiling - remaining LLVM changes

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/IR/IntrinsicInst.h:377
@@ +376,3 @@
+  /// This represents the llvm.instrprof_value_profile_target intrinsic.
+  class InstrProfValueProfileTargetInst : public IntrinsicInst {
+  public:
----------------
Suggest dropping 'Target' in the name. Just InstrProfValueProfileInstr. Target is strongly tied to indirect call value profiling.

================
Comment at: include/llvm/IR/IntrinsicInst.h:403
@@ +402,3 @@
+
+    ConstantInt *getIndex() const {
+      return cast<ConstantInt>(const_cast<Value *>(getArgOperand(4)));
----------------
Add  a comment: returns value site index.

================
Comment at: include/llvm/IR/Intrinsics.td:325
@@ +324,3 @@
+// through instrumentation based profiling.
+def int_instrprof_value_profile_target : Intrinsic<[],
+                                                   [llvm_ptr_ty, llvm_i64_ty,
----------------
Suggest drop 'target'.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:172
@@ -159,1 +171,3 @@
   const char *ProfileEnd;
+  const uint32_t *CurrentVDataCounts;
+  const ValueData *CurrentVData;
----------------
Realistically it is not useful to track more than 10 values per value site. For this reason, please change the VDataCount type to be unsigned char.

================
Comment at: lib/ProfileData/InstrProfReader.cpp:235
@@ -234,1 +234,3 @@
   auto NamesSize = swap(Header.NamesSize);
+  auto PaddingSize = sizeof(uint64_t) - NamesSize % sizeof(uint64_t);
+  auto ValueDataCountsSize = swap(Header.ValueDataCountsSize);
----------------
Please draw a diagram describing raw data layout here:

Header:
  ....
prf_data_section:
   ....
prf_cnts_section:
   ...
prf_name_section:
    ...
padding1
vdata_counts_section: // # of counts per site
  ...
padding2
vdata_value_section:
   ...


================
Comment at: lib/ProfileData/InstrProfReader.cpp:333
@@ +332,3 @@
+      InstrProfValueSiteRecord NewVRecord;
+      for (uint32_t VIndex = 0; VIndex < CurrentVDataCount; ++VIndex) {
+        uint64_t TargetValue = swap(CurrentVData->Value);
----------------
Should CurrentVData be set to Data->Values?

Current implementation seems to assume that VData and VDataCounts are laid out in the same order as the prf_data wrt the owning functions. I don't think this assumption is right.




http://reviews.llvm.org/D13308