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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:149
 
-    BoundaryPoint() : BeginCount(0), EndCount(0){};
+    bool isZeroRangeBegin;
 
----------------
hoy wrote:
> Add a comment for this? Does it stand for a point that only have a zero count?
Comments added, not a zero count only, it's also used for the overlapping point. like
```
[----10 ----]
[--------- 0 ----------]
A            B         C
```
Point A's count is 10 and the `isZeroRangeBegin` is true.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:216
     uint64_t Count = Item.second;
     if (Boundaries.find(Begin) == Boundaries.end())
       Boundaries[Begin] = BoundaryPoint();
----------------
hoy wrote:
> This initialization is probably not needed since you are adding the default constructor.
Sounds good, refactor the code to use `emplace`


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:251
+      if (Count == 0 && ZeroRangeDepth == 0)
+        BeginAddress = UINT64_MAX;
     }
----------------
hoy wrote:
> Add a comment for this? Suppose we never have a zero-count range, does this change the default behavior?
Comment added. It will change our previous logic a little bit, i, e. previous one can generate new zero-count range.
For example, supposing we have two non-overlapping ranges
```
[<---- 10 ---->]  
                            [<----20 ----->]
A              B             C             D
```

Previously, it will add a zero range [B+1, C-1]. (Anyway, this extra zero range will be filtered later as you can see the code below).

After this change, the begin point B+1 is marked invalid(UINT64_MAX), so no extra zero range is added.





================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:273
-    // doesn't belong to current context, filter them out.
-    if (Count == 0)
-      continue;
----------------
hoy wrote:
> 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.
See the above comment. that's from the gap between two non-overlapping ranges.


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


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:391
 
-  FuncStartAddrMap[StartOffset] = Symbols[SI].Name.str();
+  FuncStartAddrMap[StartOffset] = {Symbols[SI].Name.str(), EndOffset};
   return true;
----------------
hoy wrote:
> Does the initializer list require c++14? `emplace_back` should work if you see a warning for that.
The value here is a `std::pair`, I guess you mean `emplace`?


================
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.
----------------
hoy wrote:
> Nit: rename `FuncStartAddrMap` to `FuncStartOffsetMap`?
> 
> Please adjust comment to include end offset.
Good catch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109713