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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:499
     uint64_t EndOffset = I.second;
     Binary->computeInlinedContextSizeForRange(StartOffset, EndOffset);
   }
----------------
>From the way ranges of a dwarf symbol are populated, `EndOffset` should not be a part of a range (exclusive). But `computeInlinedContextSizeForRange` expects the opposite.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:336
                     : SectionOffset + SectSize;
-  if (StartOffset >= NextStartOffset)
+  if (StartOffset > NextStartOffset)
     return true;
----------------
why remove the equal compare? A label at the end of the text section has StartOffset == NextStartOffset.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:570
+        uint64_t FunctionStart = Range.LowPC;
+        uint64_t FunctionSize = Range.HighPC - FunctionStart;
+
----------------
wlei wrote:
> hoy wrote:
> > Is `HighPC` inclusive or not?
> it's inclusive, it's the same as the previous EndOffset in symbol table.
I see. It actually means exclusive which means HighPC does not belong to the current function :). Otherwise `FunctionSize` should be `Range.HighPC - FunctionStart + 1`.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:182
+  // A vector of DWARF ranges used to quick lookup for all the ranges from
+  // different part of function.
+  std::vector<DWARFRangesVectorTy> AllDWARFRanges;
----------------
nit: different parts of a function


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112282