[PATCH] D112282: [llvm-profgen] Switch to DWARF-based symbol and ranges
Lei Wang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Oct 28 13:15:32 PDT 2021
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
More information about the llvm-commits
mailing list