[llvm][NFC] Simplify ProfileSummaryInfo state transitions.

[mtrofin]

Summary

ProfileSummaryInfo is updated seldom, as result of very specific
triggers. This patch clearly demarcates state updates from read-only uses.
This, arguably, improves readability and maintainability.

[dblaikie]

Could you separate all the constification (& possibly/as much of the renaming as possible) from the semantic changes here?

(no need to send the constification for pre-commit review - feel free to commit it before or after the semantic changes depending on what suits/is practical/etc)

llvm/include/llvm/Analysis/ProfileSummaryInfo.h

68

I'd probably write this as Summary != nullptr?

[davidxl]

There might be a reason for lazy computation of the summary/thresholds data. +Hiroshi

llvm/include/llvm/Analysis/ProfileSummaryInfo.h

44

may be better to keep this helper for readability.

llvm/lib/Analysis/ProfileSummaryInfo.cpp

103–104

I think the original early return is clearer.

[hjyamauchi]

In D79920#2039115, @davidxl wrote:

There might be a reason for lazy computation of the summary/thresholds data. +Hiroshi

I'm not sure why.

In D79920#2039086, @dblaikie wrote:

Could you separate all the constification (& possibly/as much of the renaming as possible) from the semantic changes here?

+1

llvm/include/llvm/Analysis/ProfileSummaryInfo.h

44

+1

64

samples -> summary?

65

refresh() won't read again once once a non-null summary is created.

Do we anticipate a situation where the profile summary in the IR may be set once and then updated later perhaps conditionally? Who should call refresh?

llvm/lib/Transforms/IPO/SampleProfile.cpp

1851

This if statement may suggest that we may not always set it here or may have been set at an earlier point (do we know if that's possible)?

Should we move the refresh call inside the if statement?

[mtrofin]

feedback

llvm/include/llvm/Analysis/ProfileSummaryInfo.h

65

That's currently the case, too - see the old computeSummary() def and uses.

Re. the second question (if we anticipate a situation...) - my goal with this patch is to help make such analysis simpler, by clarifying where the transitions happen. When such a scenario appears, the agent invalidating the profile should trigger the refresh.

I think, ideally, we'd control the state of the PSI from the pass pipeline through explicit pass calls - it'd make it even more clear what the state is at any given moment. This patch is a step in that direction.

llvm/lib/Analysis/ProfileSummaryInfo.cpp

103–104

We still need to go and compute the thresholds.

llvm/lib/Transforms/IPO/SampleProfile.cpp

1851

That's more clear, indeed.

[hjyamauchi]

I still think it'd be best to have the pure formatting changes (clang-format) in a separate patch.

llvm/include/llvm/Analysis/ProfileSummaryInfo.h

65

I think there are two approaches here:

1. We have a single global point where PSI is filled in. (this might potentially require coordination between different parts of the code.)

2. We can have multiple points during the pass pipeline where PSI is filled in or updated. (just update PSI as we go and change the metadata.)

Which are we going with?

If 1, would it be useful to make calling refresh again after it's filled into an assert error? Currently it gets dropped silently, but it would catch if someone updates the PS in the IR and tries to update the PSI, but not really updating it.

If 2, would it be useful to make refresh update PSI every time it gets called? (which we could do for 1 as well?)

[mtrofin]

In D79920#2047502, @yamauchi wrote:

I still think it'd be best to have the pure formatting changes (clang-format) in a separate patch.

I think it's a matter of what scenarios we have: do we have a scenario for ever re-updating the PSI or not.

We do need to work that out, and that's my intention. In the meantime, I believe it would be helpful to simplify the current state of the code. As this patch clarifies state transitions, it should be relatively easy to further simplify it further afterwards.

[mtrofin]

removed unrelated clang-format changes

[mtrofin]

https://reviews.llvm.org/rGfa3b587196dbc04e445257ae38e7906e5c0c4888

llvm/include/llvm/Analysis/ProfileSummaryInfo.h

 /// check whether a function is hot or cold.
 
 // FIXME: Provide convenience methods to determine hotness/coldness of other IR
 // units. This would require making this depend on BFI.
 class ProfileSummaryInfo {
 private:
   Module &M;
   std::unique_ptr<ProfileSummary> Summary;
-  bool computeSummary();
   void computeThresholds();
   // Count thresholds to answer isHotCount and isColdCount queries.
   Optional<uint64_t> HotCountThreshold, ColdCountThreshold;
   // True if the working set size of the code is considered huge,
   // because the number of profile counts required to reach the hot
   // percentile is above a huge threshold.
   Optional<bool> HasHugeWorkingSetSize;
   // True if the working set size of the code is considered large,
   // because the number of profile counts required to reach the hot
   // percentile is above a large threshold.
   Optional<bool> HasLargeWorkingSetSize;
   // Compute the threshold for a given cutoff.
   Optional<uint64_t> computeThreshold(int PercentileCutoff);
   // The map that caches the threshold values. The keys are the percentile
   // cutoff values and the values are the corresponding threshold values.
-  DenseMap<int, uint64_t> ThresholdCache;
+  mutable DenseMap<int, uint64_t> ThresholdCache;
 
 public:
-  ProfileSummaryInfo(Module &M) : M(M) {}
-  ProfileSummaryInfo(ProfileSummaryInfo &&Arg)
-      : M(Arg.M), Summary(std::move(Arg.Summary)) {}
+  ProfileSummaryInfo(Module &M) : M(M) { refresh(); }
+  ProfileSummaryInfo(ProfileSummaryInfo &&Arg) = default;
+
+  /// If no summary is present, attempt to refresh.
+  void refresh();
 
   /// Returns true if profile summary is available.
-  bool hasProfileSummary() { return computeSummary(); }
+  bool hasProfileSummary() const { return Summary != nullptr; }
 
   /// Returns true if module \c M has sample profile.
   bool hasSampleProfile() {
     return hasProfileSummary() &&
            Summary->getKind() == ProfileSummary::PSK_Sample;
   }
 
   /// Returns true if module \c M has instrumentation profile.

llvm/lib/Analysis/ProfileSummaryInfo.cpp

static const ProfileSummaryEntry &getEntryForPercentile(SummaryEntryVector &DS,
   if (It == DS.end())
     report_fatal_error("Desired percentile exceeds the maximum cutoff");
   return *It;
 }
 
 // The profile summary metadata may be attached either by the frontend or by
 // any backend passes (IR level instrumentation, for example). This method
 // checks if the Summary is null and if so checks if the summary metadata is now
-// available in the module and parses it to get the Summary object. Returns true
-// if a valid Summary is available.
-bool ProfileSummaryInfo::computeSummary() {
-  if (Summary)
-    return true;
+// available in the module and parses it to get the Summary object.
+void ProfileSummaryInfo::refresh() {
+  if (hasProfileSummary())
+    return;
   // First try to get context sensitive ProfileSummary.
   auto *SummaryMD = M.getProfileSummary(/* IsCS */ true);
-  if (SummaryMD) {
+  if (SummaryMD)
     Summary.reset(ProfileSummary::getFromMD(SummaryMD));
-    return true;
-  }
+
+  if (!hasProfileSummary()) {
     // This will actually return PSK_Instr or PSK_Sample summary.
     SummaryMD = M.getProfileSummary(/* IsCS */ false);
-  if (!SummaryMD)
-    return false;
+    if (SummaryMD)
       Summary.reset(ProfileSummary::getFromMD(SummaryMD));
-  return true;
+  }
+  if (!hasProfileSummary())
+    return;
+  computeThresholds();
 }
 
 Optional<uint64_t> ProfileSummaryInfo::getProfileCount(const CallBase &Call,
                                                        BlockFrequencyInfo *BFI,
                                                        bool AllowSynthetic) {
   assert((isa<CallInst>(Call) || isa<InvokeInst>(Call)) &&
          "We can only get profile count for call/invoke instruction.");
   if (hasSampleProfile()) {
if (BFI)
     return BFI->getBlockProfileCount(Call.getParent(), AllowSynthetic);
   return None;
 }
 
 /// Returns true if the function's entry is hot. If it returns false, it
 /// either means it is not hot or it is unknown whether it is hot or not (for
 /// example, no profile data is available).
 bool ProfileSummaryInfo::isFunctionEntryHot(const Function *F) {
-  if (!F || !computeSummary())
+  if (!F || !hasProfileSummary())
     return false;
   auto FunctionCount = F->getEntryCount();
   // FIXME: The heuristic used below for determining hotness is based on
   // preliminary SPEC tuning for inliner. This will eventually be a
   // convenience method that calls isHotCount.
   return FunctionCount && isHotCount(FunctionCount.getCount());
 }
 
 /// Returns true if the function contains hot code. This can include a hot
 /// function entry count, hot basic block, or (in the case of Sample PGO)
 /// hot total call edge count.
 /// If it returns false, it either means it is not hot or it is unknown
 /// (for example, no profile data is available).
 bool ProfileSummaryInfo::isFunctionHotInCallGraph(const Function *F,
                                                   BlockFrequencyInfo &BFI) {
-  if (!F || !computeSummary())
+  if (!F || !hasProfileSummary())
     return false;
   if (auto FunctionCount = F->getEntryCount())
     if (isHotCount(FunctionCount.getCount()))
       return true;
 
   if (hasSampleProfile()) {
     uint64_t TotalCallCount = 0;
     for (const auto &BB : *F)

 
 /// Returns true if the function only contains cold code. This means that
 /// the function entry and blocks are all cold, and (in the case of Sample PGO)
 /// the total call edge count is cold.
 /// If it returns false, it either means it is not cold or it is unknown
 /// (for example, no profile data is available).
 bool ProfileSummaryInfo::isFunctionColdInCallGraph(const Function *F,
                                                    BlockFrequencyInfo &BFI) {
-  if (!F || !computeSummary())
+  if (!F || !hasProfileSummary())
     return false;
   if (auto FunctionCount = F->getEntryCount())
     if (!isColdCount(FunctionCount.getCount()))
       return false;
 
   if (hasSampleProfile()) {
     uint64_t TotalCallCount = 0;
     for (const auto &BB : *F)

 bool ProfileSummaryInfo::isFunctionHotnessUnknown(const Function &F) {
   assert(hasPartialSampleProfile() && "Expect partial sample profile");
   return !F.getEntryCount().hasValue();
 }
 
 template<bool isHot>
 bool ProfileSummaryInfo::isFunctionHotOrColdInCallGraphNthPercentile(
     int PercentileCutoff, const Function *F, BlockFrequencyInfo &BFI) {
-  if (!F || !computeSummary())
+  if (!F || !hasProfileSummary())
     return false;
   if (auto FunctionCount = F->getEntryCount()) {
     if (isHot &&
         isHotCountNthPercentile(PercentileCutoff, FunctionCount.getCount()))
       return true;
     if (!isHot &&
         !isColdCountNthPercentile(PercentileCutoff, FunctionCount.getCount()))
       return false;

 /// Returns true if the function's entry is a cold. If it returns false, it
 /// either means it is not cold or it is unknown whether it is cold or not (for
 /// example, no profile data is available).
 bool ProfileSummaryInfo::isFunctionEntryCold(const Function *F) {
   if (!F)
     return false;
   if (F->hasFnAttribute(Attribute::Cold))
     return true;
-  if (!computeSummary())
+  if (!hasProfileSummary())
     return false;
   auto FunctionCount = F->getEntryCount();
   // FIXME: The heuristic used below for determining coldness is based on
   // preliminary SPEC tuning for inliner. This will eventually be a
   // convenience method that calls isHotCount.
   return FunctionCount && isColdCount(FunctionCount.getCount());
 }
 
 /// Compute the hot and cold thresholds.
 void ProfileSummaryInfo::computeThresholds() {
-  if (!computeSummary())
-    return;
   auto &DetailedSummary = Summary->getDetailedSummary();
   auto &HotEntry =
       getEntryForPercentile(DetailedSummary, ProfileSummaryCutoffHot);
   HotCountThreshold = HotEntry.MinCount;
   if (ProfileSummaryHotCount.getNumOccurrences() > 0)
     HotCountThreshold = ProfileSummaryHotCount;
   auto &ColdEntry =
       getEntryForPercentile(DetailedSummary, ProfileSummaryCutoffCold);
   ColdCountThreshold = ColdEntry.MinCount;
   if (ProfileSummaryColdCount.getNumOccurrences() > 0)
     ColdCountThreshold = ProfileSummaryColdCount;
   assert(ColdCountThreshold <= HotCountThreshold &&
          "Cold count threshold cannot exceed hot count threshold!");
   HasHugeWorkingSetSize =
       HotEntry.NumCounts > ProfileSummaryHugeWorkingSetSizeThreshold;
   HasLargeWorkingSetSize =
       HotEntry.NumCounts > ProfileSummaryLargeWorkingSetSizeThreshold;
 }
 
 Optional<uint64_t> ProfileSummaryInfo::computeThreshold(int PercentileCutoff) {
-  if (!computeSummary())
+  if (!hasProfileSummary())
     return None;
   auto iter = ThresholdCache.find(PercentileCutoff);
   if (iter != ThresholdCache.end()) {
     return iter->second;
   }
   auto &DetailedSummary = Summary->getDetailedSummary();
   auto &Entry =
       getEntryForPercentile(DetailedSummary, PercentileCutoff);
   uint64_t CountThreshold = Entry.MinCount;
   ThresholdCache[PercentileCutoff] = CountThreshold;
   return CountThreshold;
 }
 
 bool ProfileSummaryInfo::hasHugeWorkingSetSize() {
-  if (!HasHugeWorkingSetSize)
-    computeThresholds();
   return HasHugeWorkingSetSize && HasHugeWorkingSetSize.getValue();
 }
 
 bool ProfileSummaryInfo::hasLargeWorkingSetSize() {
-  if (!HasLargeWorkingSetSize)
-    computeThresholds();
   return HasLargeWorkingSetSize && HasLargeWorkingSetSize.getValue();
 }
 
 bool ProfileSummaryInfo::isHotCount(uint64_t C) {
-  if (!HotCountThreshold)
-    computeThresholds();
   return HotCountThreshold && C >= HotCountThreshold.getValue();
 }
 
 bool ProfileSummaryInfo::isColdCount(uint64_t C) {
-  if (!ColdCountThreshold)
-    computeThresholds();
   return ColdCountThreshold && C <= ColdCountThreshold.getValue();
 }
 
 template<bool isHot>
 bool ProfileSummaryInfo::isHotOrColdCountNthPercentile(int PercentileCutoff,
                                                        uint64_t C) {
   auto CountThreshold = computeThreshold(PercentileCutoff);
   if (isHot)
     return CountThreshold && C >= CountThreshold.getValue();
   else
     return CountThreshold && C <= CountThreshold.getValue();
 }
 
 bool ProfileSummaryInfo::isHotCountNthPercentile(int PercentileCutoff, uint64_t C) {
   return isHotOrColdCountNthPercentile<true>(PercentileCutoff, C);
 }
 
 bool ProfileSummaryInfo::isColdCountNthPercentile(int PercentileCutoff, uint64_t C) {
   return isHotOrColdCountNthPercentile<false>(PercentileCutoff, C);
 }
 
 uint64_t ProfileSummaryInfo::getOrCompHotCountThreshold() {
-  if (!HotCountThreshold)
-    computeThresholds();
   return HotCountThreshold ? HotCountThreshold.getValue() : UINT64_MAX;
 }
 
 uint64_t ProfileSummaryInfo::getOrCompColdCountThreshold() {
-  if (!ColdCountThreshold)
-    computeThresholds();
   return ColdCountThreshold ? ColdCountThreshold.getValue() : 0;
 }
 
 bool ProfileSummaryInfo::isHotBlock(const BasicBlock *BB, BlockFrequencyInfo *BFI) {
   auto Count = BFI->getBlockProfileCount(BB);
   return Count && isHotCount(*Count);
 }
 

llvm/lib/Transforms/IPO/SampleProfile.cpp

 
 bool SampleProfileLoader::runOnModule(Module &M, ModuleAnalysisManager *AM,
                                       ProfileSummaryInfo *_PSI, CallGraph *CG) {
   if (!ProfileIsValid)
     return false;
   GUIDToFuncNameMapper Mapper(M, *Reader, GUIDToFuncNameMap);
 
   PSI = _PSI;
-  if (M.getProfileSummary(/* IsCS */ false) == nullptr)
+  if (M.getProfileSummary(/* IsCS */ false) == nullptr) {
     M.setProfileSummary(Reader->getSummary().getMD(M.getContext()),
                         ProfileSummary::PSK_Sample);
-
+    PSI->refresh();
+  }
   // Compute the total number of samples collected in this profile.
   for (const auto &I : Reader->getProfiles())
     TotalCollectedSamples += I.second.getTotalSamples();
 
   // Populate the symbol map.
   for (const auto &N_F : M.getValueSymbolTable()) {
     StringRef OrigName = N_F.getKey();
     Function *F = dyn_cast<Function>(N_F.getValue());

llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

static bool annotateAllFunctions(
   }
 
   // Add the profile summary (read from the header of the indexed summary) here
   // so that we can use it below when reading counters (which checks if the
   // function should be marked with a cold or inlinehint attribute).
   M.setProfileSummary(PGOReader->getSummary(IsCS).getMD(M.getContext()),
                       IsCS ? ProfileSummary::PSK_CSInstr
                            : ProfileSummary::PSK_Instr);
+  PSI->refresh();
 
   std::unordered_multimap<Comdat *, GlobalValue *> ComdatMembers;
   collectComdatMembers(M, ComdatMembers);
   std::vector<Function *> HotFunctions;
   std::vector<Function *> ColdFunctions;
   for (auto &F : M) {
     if (F.isDeclaration())
       continue;