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

[Wenlei He via Phabricator via llvm-commits]

wenlei 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;
+};
----------------
nit: IsFunctionEntry->isFuncEntry to be consistent.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:188
+  // FuncRange's RangesIdx.
+  std::vector<RangesTy> FuncRangesVec;
+
----------------
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.


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:190
+
+  std::set<std::string> FuncSymNames;
+
----------------
Would rellocate invalidate the StringRef holding the underlying strings? 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:371
+
+  RangesTy &getAllRangesOfOneFunc(FuncRange *FRange) {
+    return FuncRangesVec[FRange->RangesIdx];
----------------
nit: getAllRangesOfOneFunc -> getAllRangesForFunc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112282