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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/noinline-noprobe.test:12
+CHECK:  2: 19
+CHECK:  3: 19 bar:21
+CHECK: bar:324:21
----------------
wenlei wrote:
> We may want to strengthen the tests a bit since breaking llvm-profgen would be quite impactful for AutoFDO performance. 
> 
> - Add a test to cover call site profile with multiple targets? 
> - Test discriminator: we have correct base discriminator as well as duplication factor in the generated profile.
> - Different profile format, extended binary format has profiled symbol list (it's ok to deal with symbol list later and separately)
> - Cover these cases for nested inlinee profile too.  
added a large test case (quick_sort), it will cover the call site with multiple targets and nested inlinee profile.

> Test discriminator: we have correct base discriminator as well as duplication factor in the generated profile
I had a standalone test case for discriminator test , see https://reviews.llvm.org/D109934

>Different profile format, extended binary format has profiled symbol list (it's ok to deal with symbol list later and separately)
Sounds good, will add it shortly.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:320
+    FunctionSamples &CalleeProfile = getTopLevelFunctionProfile(CalleeName);
+    assert(Count != 0 && "Unexpected zero weight branch");
+    CalleeProfile.addHeadSamples(Count);
----------------
wenlei wrote:
> move this assertion to the top? 
fixed.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:54-55
+                                   uint64_t Count);
+  void populateFunctionBodySamples(const RangeSample &RangeCounter);
+  void populateFunctionBoundarySamples(const BranchSample &BranchCounters);
+
----------------
wenlei wrote:
> 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`?
Sounds good!


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:25
 
 class ProfileGenerator {
 
----------------
wenlei wrote:
> 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?
Good point, four sub-class is really harder to maintain and extend, refactored.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109551