[PATCH] D30053: Add function importing info from samplepgo profile to the module summary.

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:188
+    CallGraphEdges[I].updateHotness(CalleeInfo::HotnessType::Hot);
+
   bool NonRenamableLocal = isNonRenamableLocal(F);
----------------
danielcdh wrote:
> mehdi_amini wrote:
> > danielcdh wrote:
> > > mehdi_amini wrote:
> > > > danielcdh wrote:
> > > > > mehdi_amini wrote:
> > > > > > I'm still unsure why we need this side channel to communicate the hotness, while we have above already some infrastructure? Why isn't samplePGO integrate in the general PGO infrastructure?
> > > > > The root cause is the profile annotation mechanism is different.
> > > > > 
> > > > > SamplePGO needs to make sure that before profile annotation, the IR resembles the hot portion of the profiling binary. As a result, we need to explicitly inline functions before profile annotation. But in order to inline, we need to first have it imported.
> > > > > 
> > > > > I can go into more details with an example if you want. Please let me know.
> > > > I'm still not fond of this "side channel", and I rather have a solution that annotates hot calls in the IR instead, that would be agnostic to the source of information we derive this "hotness" from.
> > > I think we already have a side channel for ICP?
> > > 
> > > The issue for marking "hot call" is that it only goes down by 1 level, for the cases where foo_in_a_cc()->bar_in_b_cc()->baz_in_c_cc(), we may only go one level to include bar_in_b_cc if we mark the hot call. But in practice, we need both levels to be imported and inlined.
> > I don't see how having one level at a time is an issue. The two calls can be marked hot and considered independently when building the chain.
> The issue is, when compiling bar_in_b_cc(), the profile for foo_in_a_cc() is not visible. If the inline instance is the only instance for bar_in_b_cc, then the standalone copy of bar_in_b_cc may not have profile and is considered cold and thus may not be able to mark the callsite to baz_in_c_cc as hot.
OK it is getting more clear: it is interesting because it means we have some sort of "context sensitivity" : we're not recording that `bar_in_b_cc()->baz_in_c_cc()` is hot in the absolute, but only when called from `foo_in_a_cc()`.

It is still not great to add edges in the call graph to model this. You not recording the information described above IIUC, but you're recording a "hot call" from `foo_in_a_cc()` to `bar_in_b_cc()`. I'd have to think about possible unintended consequences from what it means for the summary representation and the analyses.


https://reviews.llvm.org/D30053