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

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.

lgtm, except two nits. thanks..



================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:188
+  // FuncRange's RangesIdx.
+  std::vector<RangesTy> FuncRangesVec;
+
----------------
wlei wrote:
> 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?
Have a slight preference towards BinaryFunction (given it reflects the binary representation of the function, in terms of address ranges). Not a big deal though.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:82
+  size_t RangesIdx;
+  uint64_t EndOffset;
+};
----------------
wlei wrote:
> wenlei wrote:
> > As struct representation for a symbol, having EndOffset only but not start offset seem weird. I think we either "complete" it as a general symbol definition, or rename it to make it explicit that this is just some special purpose struct to specific bookkeeping map. 
> > 
> > Also please comment if the end offset is inclusive or exclusive. 
> I added some comments to clarify this, i, e. this struct is always accessed by `StartOffset2ElfRangeInfoMap` whose key is the start offset so just skip it here. See if this's clearer, I'm also fine to add the "StartOffset" for good readability.
Now that we have a proper Function abstraction, I think it's better to have the start offset field for the ranges, so the structure is more complete.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112282