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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
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.
----------------
hoy wrote:
> typo: leverange 
Good catch, thanks!


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:570
+        uint64_t FunctionStart = Range.LowPC;
+        uint64_t FunctionSize = Range.HighPC - FunctionStart;
+
----------------
hoy wrote:
> Is `HighPC` inclusive or not?
it's inclusive, it's the same as the previous EndOffset in symbol table.


================
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);
----------------
hoy wrote:
> nit: getSymbolForStartOffset -> getDwarfSymbolForStartOffset ? Similar to findSymbolForOffset below.
Fixed.


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

Do you mean to assert (Offset <= EndOffset & "..."); ? This sounds a good place, with this, we don't need https://reviews.llvm.org/D111902 then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112282