[PATCH] D109551: [AutoFDO][llvm-profgen] Profile generation for LBR(non-CS) sample

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:54-55
+                                   uint64_t Count);
+  void populateFunctionBodySamples(const RangeSample &RangeCounter);
+  void populateFunctionBoundarySamples(const BranchSample &BranchCounters);
+
----------------
I was initially a bit confused about why these are not virtual since CSProfileGenerator defines them too. Then I found they have different params (still confused why).. Now I understand here it's for all functions, but in CSProfileGenerator it's for one function. Perhaps we can name them `populateBodySamplesForFunction` vs `populateBodySamplesForAllFunctions`? Same for `populateFunctionBoundarySamples`?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:25
 
 class ProfileGenerator {
 
----------------
hoy wrote:
> Since we are now using this base class to generate non-CS non-probe profile, add a header comment to clarify that?
I am thinking about whether profile generation should follow the structure of perf reader. It looks like there's not much overlap for CS and non-CS profile as the profile format is different -pretty much updateBodySamplesforFunctionProfile is the only function from the base that is being reused? If that's the case, would it be better to have a separate ProfileGenerator base as interface then two subclass, one for CS one for non-CS?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109551