[SampleFDO][NFC] Detach SampleProfileLoader from SampleCoverageTracker

[xur]

Summary

his is split from patch: https://reviews.llvm.org/D95698

This patch detaches SampleProfileLoader from SampleCoverageTracker. We plan to move SampleProfileLoader to a template class. This would keep
SampleCoverageTracker as a class. Also make callsiteIsHot() as a file static function.

[davidxl]

Why making callSiteIsHot a file static func?

[xur]

It's only used in this file, by SampleProfileLoader and SampleProfileTracker class. I could make it a class static method (in SampleProfileLoader), But I don't see an advantage of that -- just with longer class qualifier.

[wmi]

This patch detaches SampleProfileLoader from SampleCoverageTracker. This would keep SampleCoverageTracker as a class.

It means you don't want SampleCoverageTracker to be a template. Will the FS profile annotation in machine code level also need some coverage tracker?

[xur]

Machine level profile loader will use the SampleCoverage Tracker. This
class will be kept in the templated header. Machine level profile loader
and IR level profile loader will be using the exactly the same class (not
templatelized).

[wmi]

In D95823#2535255, @xur wrote:

Machine level profile loader will use the SampleCoverage Tracker. This
class will be kept in the templated header. Machine level profile loader
and IR level profile loader will be using the exactly the same class (not
templatelized).

I see. During profile annotation in machine IR level, for FunctionSamples only using base discriminator, will they be used for annotation again or their annotation will be skipped? I am asking because I am wondering what the coverage report will look like for SampleCoverageTracker used in machine IR.

[wmi]

LGTM.

llvm/lib/Transforms/IPO/SampleProfile.cpp

 using EdgeWeightMap = DenseMap<Edge, uint64_t>;
 using BlockEdgeMap =
     DenseMap<const BasicBlock *, SmallVector<const BasicBlock *, 8>>;
 
 class SampleProfileLoader;
 
 class SampleCoverageTracker {
 public:
-  SampleCoverageTracker(SampleProfileLoader &SPL) : SPLoader(SPL){};
-
   bool markSamplesUsed(const FunctionSamples *FS, uint32_t LineOffset,
                        uint32_t Discriminator, uint64_t Samples);
   unsigned computeCoverage(unsigned Used, unsigned Total) const;
   unsigned countUsedRecords(const FunctionSamples *FS,
                             ProfileSummaryInfo *PSI) const;
   unsigned countBodyRecords(const FunctionSamples *FS,
                             ProfileSummaryInfo *PSI) const;
   uint64_t getTotalUsedSamples() const { return TotalUsedSamples; }
   uint64_t countBodySamples(const FunctionSamples *FS,
                             ProfileSummaryInfo *PSI) const;
 
   void clear() {
     SampleCoverage.clear();
     TotalUsedSamples = 0;
   }
+  inline void setProfAccForSymsInList(bool V) { ProfAccForSymsInList = V; }
 
 private:
   using BodySampleCoverageMap = std::map<LineLocation, unsigned>;
   using FunctionSamplesCoverageMap =
       DenseMap<const FunctionSamples *, BodySampleCoverageMap>;
 
   /// Coverage map for sampling records.
   ///
private:
   /// its callsites.
   ///
   /// Note that this accumulator tracks samples used from a single function
   /// and all the inlined callsites. Strictly, we should have a map of counters
   /// keyed by FunctionSamples pointers, but these stats are cleared after
   /// every function, so we just need to keep a single counter.
   uint64_t TotalUsedSamples = 0;
 
-  SampleProfileLoader &SPLoader;
+  // For symbol in profile symbol list, whether to regard their profiles
+  // to be accurate. This is passed from the SampleLoader instance.
+  bool ProfAccForSymsInList = false;
 };
 
 class GUIDToFuncNameMapper {
 public:
   GUIDToFuncNameMapper(Module &M, SampleProfileReader &Reader,
                         DenseMap<uint64_t, StringRef> &GUIDToFuncNameMap)
       : CurrentReader(Reader), CurrentModule(M),
       CurrentGUIDToFuncNameMap(GUIDToFuncNameMap) {

 public:
   SampleProfileLoader(
       StringRef Name, StringRef RemapName, ThinOrFullLTOPhase LTOPhase,
       std::function<AssumptionCache &(Function &)> GetAssumptionCache,
       std::function<TargetTransformInfo &(Function &)> GetTargetTransformInfo,
       std::function<const TargetLibraryInfo &(Function &)> GetTLI)
       : GetAC(std::move(GetAssumptionCache)),
         GetTTI(std::move(GetTargetTransformInfo)), GetTLI(std::move(GetTLI)),
-        CoverageTracker(*this), Filename(std::string(Name)),
-        RemappingFilename(std::string(RemapName)), LTOPhase(LTOPhase) {}
+        Filename(std::string(Name)), RemappingFilename(std::string(RemapName)),
+        LTOPhase(LTOPhase) {}
 
   bool doInitialization(Module &M, FunctionAnalysisManager *FAM = nullptr);
   bool runOnModule(Module &M, ModuleAnalysisManager *AM,
                    ProfileSummaryInfo *_PSI, CallGraph *CG);
 
   void dump() { Reader->dump(); }
 
 protected:
protected:
 
   void propagateWeights(Function &F);
   uint64_t visitEdge(Edge E, unsigned *NumUnknownEdges, Edge *UnknownEdge);
   void buildEdges(Function &F);
   std::vector<Function *> buildFunctionOrder(Module &M, CallGraph *CG);
   bool propagateThroughEdges(Function &F, bool UpdateBlockCount);
   void computeDominanceAndLoopInfo(Function &F);
   void clearFunctionData();
-  bool callsiteIsHot(const FunctionSamples *CallsiteFS,
-                     ProfileSummaryInfo *PSI);
 
   /// Map basic blocks to their computed weights.
   ///
   /// The weight of a basic block is defined to be the maximum
   /// of all the instruction weights in that block.
   BlockWeightMap BlockWeights;
 
   /// Map edges to their computed weights.

 /// sample count with the hot cutoff computed by ProfileSummaryInfo, it is
 /// regarded as hot if the count is above the cutoff value.
 ///
 /// When ProfileAccurateForSymsInList is enabled and profile symbol list
 /// is present, functions in the profile symbol list but without profile will
 /// be regarded as cold and much less inlining will happen in CGSCC inlining
 /// pass, so we tend to lower the hot criteria here to allow more early
 /// inlining to happen for warm callsites and it is helpful for performance.
-bool SampleProfileLoader::callsiteIsHot(const FunctionSamples *CallsiteFS,
-                                        ProfileSummaryInfo *PSI) {
+static bool callsiteIsHot(const FunctionSamples *CallsiteFS,
+                          ProfileSummaryInfo *PSI, bool ProfAccForSymsInList) {
   if (!CallsiteFS)
     return false; // The callsite was not inlined in the original binary.
 
   assert(PSI && "PSI is expected to be non null");
   uint64_t CallsiteTotalSamples = CallsiteFS->getTotalSamples();
   if (ProfAccForSymsInList)
     return !PSI->isColdCount(CallsiteTotalSamples);
   else
SampleCoverageTracker::countUsedRecords(const FunctionSamples *FS,
   unsigned Count = (I != SampleCoverage.end()) ? I->second.size() : 0;
 
   // If there are inlined callsites in this function, count the samples found
   // in the respective bodies. However, do not bother counting callees with 0
   // total samples, these are callees that were never invoked at runtime.
   for (const auto &I : FS->getCallsiteSamples())
     for (const auto &J : I.second) {
       const FunctionSamples *CalleeSamples = &J.second;
-      if (SPLoader.callsiteIsHot(CalleeSamples, PSI))
+      if (callsiteIsHot(CalleeSamples, PSI, ProfAccForSymsInList))
         Count += countUsedRecords(CalleeSamples, PSI);
     }
 
   return Count;
 }
 
 /// Return the number of sample records in the body of this profile.
 ///
 /// This count does not include records from cold inlined callsites.
 unsigned
 SampleCoverageTracker::countBodyRecords(const FunctionSamples *FS,
                                         ProfileSummaryInfo *PSI) const {
   unsigned Count = FS->getBodySamples().size();
 
   // Only count records in hot callsites.
   for (const auto &I : FS->getCallsiteSamples())
     for (const auto &J : I.second) {
       const FunctionSamples *CalleeSamples = &J.second;
-      if (SPLoader.callsiteIsHot(CalleeSamples, PSI))
+      if (callsiteIsHot(CalleeSamples, PSI, ProfAccForSymsInList))
         Count += countBodyRecords(CalleeSamples, PSI);
     }
 
   return Count;
 }
 
 /// Return the number of samples collected in the body of this profile.
 ///
 /// This count does not include samples from cold inlined callsites.
 uint64_t
 SampleCoverageTracker::countBodySamples(const FunctionSamples *FS,
                                         ProfileSummaryInfo *PSI) const {
   uint64_t Total = 0;
   for (const auto &I : FS->getBodySamples())
     Total += I.second.getSamples();
 
   // Only count samples in hot callsites.
   for (const auto &I : FS->getCallsiteSamples())
     for (const auto &J : I.second) {
       const FunctionSamples *CalleeSamples = &J.second;
-      if (SPLoader.callsiteIsHot(CalleeSamples, PSI))
+      if (callsiteIsHot(CalleeSamples, PSI, ProfAccForSymsInList))
         Total += countBodySamples(CalleeSamples, PSI);
     }
 
   return Total;
 }
 
 /// Return the fraction of sample records used in this profile.
 ///
for (auto &BB : F) {
         const FunctionSamples *FS = nullptr;
         if (auto *CB = dyn_cast<CallBase>(&I)) {
           if (!isa<IntrinsicInst>(I) && (FS = findCalleeFunctionSamples(*CB))) {
             assert((!FunctionSamples::UseMD5 || FS->GUIDToFuncNameMap) &&
                    "GUIDToFuncNameMap has to be populated");
             AllCandidates.push_back(CB);
             if (FS->getEntrySamples() > 0 || ProfileIsCS)
               LocalNotInlinedCallSites.try_emplace(CB, FS);
-            if (callsiteIsHot(FS, PSI))
+            if (callsiteIsHot(FS, PSI, ProfAccForSymsInList))
               Hot = true;
             else if (shouldInlineColdCallee(*CB))
               ColdCandidates.push_back(CB);
           }
         }
       }
       if (Hot || ExternalInlineAdvisor) {
         CIS.insert(CIS.begin(), AllCandidates.begin(), AllCandidates.end());
for (CallBase *I : CIS) {
         uint64_t Sum;
         for (const auto *FS : findIndirectCallFunctionSamples(*I, Sum)) {
           uint64_t SumOrigin = Sum;
           if (LTOPhase == ThinOrFullLTOPhase::ThinLTOPreLink) {
             FS->findInlinedFunctions(InlinedGUIDs, F.getParent(),
                                      PSI->getOrCompHotCountThreshold());
             continue;
           }
-          if (!callsiteIsHot(FS, PSI))
+          if (!callsiteIsHot(FS, PSI, ProfAccForSymsInList))
             continue;
 
           Candidate = {I, FS, FS->getEntrySamples(), 1.0};
           if (tryPromoteAndInlineCandidate(F, Candidate, SumOrigin, Sum,
                                            PromotedInsns)) {
             LocalNotInlinedCallSites.erase(I);
             LocalChanged = true;
           }
bool SampleProfileLoader::doInitialization(Module &M,
 
   // While profile-sample-accurate is on, ignore symbol list.
   ProfAccForSymsInList =
       ProfileAccurateForSymsInList && PSL && !ProfileSampleAccurate;
   if (ProfAccForSymsInList) {
     NamesInProfile.clear();
     if (auto NameTable = Reader->getNameTable())
       NamesInProfile.insert(NameTable->begin(), NameTable->end());
+    CoverageTracker.setProfAccForSymsInList(true);
   }
 
   if (FAM && !ProfileInlineReplayFile.empty()) {
     ExternalInlineAdvisor = std::make_unique<ReplayInlineAdvisor>(
         M, *FAM, Ctx, /*OriginalAdvisor=*/nullptr, ProfileInlineReplayFile,
         /*EmitRemarks=*/false);
     if (!ExternalInlineAdvisor->areReplayRemarksLoaded())
       ExternalInlineAdvisor.reset();
bool SampleProfileLoader::runOnFunction(Function &F, ModuleAnalysisManager *AM) {
   if (ProfileSampleAccurate || F.hasFnAttribute("profile-sample-accurate")) {
     // initialize all the function entry counts to 0. It means all the
     // functions without profile will be regarded as cold.
     initialEntryCount = 0;
     // profile-sample-accurate is a user assertion which has a higher precedence
     // than symbol list. When profile-sample-accurate is on, ignore symbol list.
     ProfAccForSymsInList = false;
   }
+  CoverageTracker.setProfAccForSymsInList(ProfAccForSymsInList);
 
   // PSL -- profile symbol list include all the symbols in sampled binary.
   // If ProfileAccurateForSymsInList is enabled, PSL is used to treat
   // old functions without samples being cold, without having to worry
   // about new and hot functions being mistakenly treated as cold.
   if (ProfAccForSymsInList) {
     // Initialize the entry count to 0 for functions in the list.
     if (PSL->contains(F.getName()))