[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/ProfiledBinary.h:87
+  // Whether the start offset is the real entry of the function.
+  bool IsFunctionEntry = false;
+};
----------------
wenlei wrote:
> nit: IsFunctionEntry->isFuncEntry to be consistent.
Fixed,thanks!


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:188
+  // FuncRange's RangesIdx.
+  std::vector<RangesTy> FuncRangesVec;
+
----------------
wenlei wrote:
> Instead of using index to access all ranges of a function, how about using symbol name directly? we could have a map (function name -> function object, and function object contains a list of ranges).
> 
> Just trying to make the scattered data a bit more structured. Seems it would be cleaner if we have a proper abstraction for Function, which contains name and list of ranges. What do you think?
> 
> If this can lead to bigger change, we can also deal with it later in a separate patch.
This is nice abstraction, not a big change, changed.

one thing is the `Function` is conflict with `llvm::Function`, then I changed to `ProfFunction`, see if this looks good?


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:190
+
+  std::set<std::string> FuncSymNames;
+
----------------
wenlei wrote:
> Would rellocate invalidate the StringRef holding the underlying strings? 
Seems we all use this way to persist data for a reference type, like the context
` std::unordered_set<SampleContextFrameVector, SampleContextFrameHash> Contexts;`. I guess this should work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112282