[PATCH] D79920: [llvm][NFC] Simplify ProfileSummaryInfo state transitions.

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ProfileSummaryInfo.h:63
+  /// If no samples were present, attempt to refresh.
+  void refresh();
 
----------------
yamauchi wrote:
> 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?
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.


================
Comment at: llvm/lib/Analysis/ProfileSummaryInfo.cpp:97
     Summary.reset(ProfileSummary::getFromMD(SummaryMD));
-    return true;
   }
+  if (!hasProfileSummary()) {
----------------
davidxl wrote:
> I think the original early return is clearer.
We still need to go and compute the thresholds.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1851
   PSI = _PSI;
   if (M.getProfileSummary(/* IsCS */ false) == nullptr)
     M.setProfileSummary(Reader->getSummary().getMD(M.getContext()),
----------------
yamauchi wrote:
> 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?
> 
That's more clear, indeed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79920