[PATCH] D28964: [PGO] Value profile support for value ranges

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

The raw profile format version needs to change too.



================
Comment at: lib/profile/InstrProfData.inc:156
 VALUE_PROF_FUNC_PARAM(void *, Data, Type::getInt8PtrTy(Ctx)) INSTR_PROF_COMMA
+#ifndef VALUE_RANGE_PROF
 VALUE_PROF_FUNC_PARAM(uint32_t, CounterIndex, Type::getInt32Ty(Ctx))
----------------
remove this line


================
Comment at: lib/profile/InstrProfData.inc:158
 VALUE_PROF_FUNC_PARAM(uint32_t, CounterIndex, Type::getInt32Ty(Ctx))
+#else /* VALUE_RANGE_PROF */
+VALUE_PROF_FUNC_PARAM(uint32_t, CounterIndex, Type::getInt32Ty(Ctx)) \
----------------
change this line to

#ifdef VALUE_RANGE_PROF


================
Comment at: lib/profile/InstrProfilingValue.c:223
 /*
+ * We divide the possible values of TargetValue into the following ranges:
+ *   Range                               Represent Value
----------------
Describe it more clearly to cover information such as: the target values are partitioned into multiple regions/ranges. There is one contiguous region which is precise -- every value in the range is tracked individually.  A value outside the precise region will be collapsed into one value depending on the region it falls in.


================
Comment at: lib/profile/InstrProfilingValue.c:228
+ * (SmallTargetalue, LargeTargetValue)   SmallTargetValue + 1
+ * [LargeTargetValue, +inf)              LargetTargetValue
+ */
----------------
There are three regions:

1) (-inf, StartVal) and (PreciseLast, LargeValue) belong to one region -- all values here should be mapped to one value
2) [PreciseStart, PreciseLast] 
3) Large values: [LargeValue, +inf) maps to one value.


================
Comment at: lib/profile/InstrProfilingValue.c:231
+COMPILER_RT_VISIBILITY void
+__llvm_profile_instrument_range(uint64_t TargetValue, void *Data,
+                                uint32_t CounterIndex, uint32_t SignedType,
----------------
for range/interval profiling, it is simpler to make target value Int64 type. With this, we can get rid of SignedType parameter.  LargeTargetValue is also optional. The default value of MIN_INT64 indicates it is not specified.


================
Comment at: lib/profile/InstrProfilingValue.c:233
+                                uint32_t CounterIndex, uint32_t SignedType,
+                                uint64_t StartValue,
+                                uint64_t SmallTargetValue,
----------------
StartValue --> PreciseRangeStart


================
Comment at: lib/profile/InstrProfilingValue.c:234
+                                uint64_t StartValue,
+                                uint64_t SmallTargetValue,
+                                uint64_t LargeTargetValue) {
----------------
PreciseRangeLast


https://reviews.llvm.org/D28964