[PATCH] D112282: [llvm-profgen] Switch to DWARF-based symbol and ranges

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

This work should improve the robustness of symbol correlation for cases where an original source function is implemented by multiple labeled binary code slices. Thanks for working on this.



================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:349
   // as unexecuted(cold).
+  // Note that a function can be spilt into multiple ranges, so we leverange
+  // DWARFSymbol's RangeIdx to look up all ranges for one entry.
----------------
typo: leverange 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:570
+        uint64_t FunctionStart = Range.LowPC;
+        uint64_t FunctionSize = Range.HighPC - FunctionStart;
+
----------------
Is `HighPC` inclusive or not?


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:341
 
-  StringRef getFuncFromStartOffset(uint64_t Offset) {
-    auto I = FuncStartOffsetMap.find(Offset);
-    if (I == FuncStartOffsetMap.end())
-      return StringRef();
-    return I->second.first;
+  DWARFSymbol *getSymbolForStartOffset(uint64_t Offset) {
+    auto I = StartOffset2DWARFSymbolMap.find(Offset);
----------------
nit: getSymbolForStartOffset -> getDwarfSymbolForStartOffset ? Similar to findSymbolForOffset below.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:355
     I--;
-    return {I->first, I->second.second};
+    return &I->second;
+  }
----------------
assert `Offset` falls into the range started by the start address?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112282