[PATCH] D111902: [llvm-profgen] Warn on invalid range and show warning summary

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1038
+    return;
+  WithColor::warning() << format("%.2f", static_cast<double>(Num) * 100 / Total)
+                       << "%(" << Num << "/" << Total
----------------
Suggest we rephrase the message.

```
warning: 0.05%(1120/2428958) cases with issue: Profile context truncated due to missing probe for call instruction.
```
->
```
warning: 0.05% (1120/2428958) of profiled contexts are truncated due to missing probe for call instruction.
```

```
warning: 0.00%(2/178637) cases with issue: Range does not belong to any functions, likely from external function.
```
->
```
warning: 0.00% (2/178637) of profiled ranges do not belong to any functions.
```

And specifically for invalid ranges, I thought we filter out external entries already, no? Did you check that they are actually from external functions?


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1081-1082
+      "Range does not belong to any functions, likely from external function.";
+  const char *RangeCrossFuncMsg =
+      "Fall through range should not cross function boundaries.";
+
----------------
For EndNotBoundaryMsg and RangeCrossFuncMsg, we can hint profile/ binary mismatch: likely due to profile and binary mismatch.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1097
+
+    auto *FRange = Binary->findFuncRangeForOffset(StartOffset);
+    if (!FRange) {
----------------
could also check `addressIsCode(StartOffset)` to make sure start is also on instruction boundary? 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:1099
+    if (!FRange) {
+      DanglingRange++;
+      WarnInvalidRange(StartOffset, EndOffset, DanglingRangeMsg);
----------------
"dangling" can be a confusing term. If this is truly external, we can say ExternalRange, or whatever that's more explicit.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:327
 
+  bool offsetIsBoundry(uint64_t Offset) {
+    return BranchOffsets.count(Offset) || RetOffsets.count(Offset) ||
----------------
nit: offsetIsTransfer - this would be consistent with others. flow transfer instructions includes call,ret,branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111902