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

[Hiroshi Yamauchi via Phabricator via llvm-commits]

yamauchi added a comment.

In D79920#2039115 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D79920#2039086>, @dblaikie wrote:

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


+1



================
Comment at: llvm/include/llvm/Analysis/ProfileSummaryInfo.h:44
-  bool computeSummary();
-  void computeThresholds();
   // Count thresholds to answer isHotCount and isColdCount queries.
----------------
davidxl wrote:
> may be better to keep this helper for readability.
+1


================
Comment at: llvm/include/llvm/Analysis/ProfileSummaryInfo.h:62
+
+  /// If no samples were present, attempt to refresh.
+  void refresh();
----------------
samples -> summary?


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


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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79920