[PATCH] D109713: [AutoFDO][llvm-profgen] Report zero count for unexecuted part of function code

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:149
 
-    BoundaryPoint() : BeginCount(0), EndCount(0){};
+    bool isZeroRangeBegin;
 
----------------
Add a comment for this? Does it stand for a point that only have a zero count?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:216
     uint64_t Count = Item.second;
     if (Boundaries.find(Begin) == Boundaries.end())
       Boundaries[Begin] = BoundaryPoint();
----------------
This initialization is probably not needed since you are adding the default constructor.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:236
     BoundaryPoint &Point = Item.second;
-    if (Point.BeginCount) {
+    if (Point.BeginCount != UINT64_MAX) {
       if (BeginAddress != UINT64_MAX)
----------------
Should this always be true? `addBeginCount` is called every time a new boundary point is created?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:273
-    // doesn't belong to current context, filter them out.
-    if (Count == 0)
-      continue;
----------------
Can you remind me of what could lead to a zero count previously? I'm wondering if that kind of zero count should be reported.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:301
+RangeSample
+ProfileGenerator::preProcessRangeCounter(const RangeSample &RangeCounter) {
+  RangeSample Ranges(RangeCounter.begin(), RangeCounter.end());
----------------
nit: preProcess -> preprocess


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:391
 
-  FuncStartAddrMap[StartOffset] = Symbols[SI].Name.str();
+  FuncStartAddrMap[StartOffset] = {Symbols[SI].Name.str(), EndOffset};
   return true;
----------------
Does the initializer list require c++14? `emplace_back` should work if you see a warning for that.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:168
   // Function offset to name mapping.
-  std::unordered_map<uint64_t, std::string> FuncStartAddrMap;
+  std::map<uint64_t, std::pair<std::string, uint64_t>> FuncStartAddrMap;
   // Offset to context location map. Used to expand the context.
----------------
Nit: rename `FuncStartAddrMap` to `FuncStartOffsetMap`?

Please adjust comment to include end offset.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109713/new/

https://reviews.llvm.org/D109713