[PATCH] D32877: Restrict call metadata based hotness detection to Sample PGO mode

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: include/llvm/Analysis/ProfileSummaryInfo.h:62
+                                            BlockFrequencyInfo *BFI,
+                                            ProfileSummary *Summary = nullptr);
   /// \brief Returns true if \p F has hot function entry.
----------------
eraman wrote:
> tejohnson wrote:
> > eraman wrote:
> > > A major rationale for adding ProfileSummaryInfo as a separate analysis is to prevent Profilesummary from being directly manipulated, so I believe we shouldn't add an interface that takes ProfileSummary as a parameter.  As Dehao mentions below, you anyway get the module from the instruction and get the kind from there, so this is not necessary. My concern there is this becomes unnecessarily expensive as we get an invariant value (kind) many times. 
> > > A major rationale for adding ProfileSummaryInfo as a separate analysis is to prevent Profilesummary from being directly manipulated, so I believe we shouldn't add an interface that takes ProfileSummary as a parameter. 
> > 
> > Note that this is only passed in when getProfileCount() is called from a ProfileSummaryInfo method (it passes in the Summary object it owns), so I am not sure what the concern is about directly manipulating it?
> > 
> > > As Dehao mentions below, you anyway get the module from the instruction and get the kind from there, so this is not necessary. My concern there is this becomes unnecessarily expensive as we get an invariant value (kind) many times.
> > 
> > Exactly, that's why I don't think we should keep re-finding the profile metadata every time when it is called from the ProfileSummaryInfo.
> > 
> > 
> The concern is you now have a public method that takes ProfileSummaryInfo * (even though no one outside the class passes it). What you can do is have a private helper method that takes a ProfileSummaryInfo *. Within the class, call this directly. Note that 
>  the class already owns the Summary. External users will still call the public method, where you can get the summary from the instruction and call the private method (with a comment about the overhead). 
Done


https://reviews.llvm.org/D32877