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

Tue Feb 28 06:43:25 PST 2017

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

LGTM

================
Comment at: docs/BranchWeightMetadata.rst:144
+the GUID of the functions that needs to be imported by ThinLTO. This is only
+set by sampling based profile. The reason that we cannot annotate this on the
+callsite is that it can only goes down 1 level in the call chain. For the cases
----------------
I would explicitly add that it is needed because the sampling based profile was collected on a binary that had already imported and inlined these functions, and we need to ensure the IR matches in the ThinLTO backends for profile annotation.

================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:188
+    CallGraphEdges[I].updateHotness(CalleeInfo::HotnessType::Hot);
+
   bool NonRenamableLocal = isNonRenamableLocal(F);
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > danielcdh wrote:
> > > mehdi_amini wrote:
> > > > 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.
> > > Yeah, context-sensitivity is one big benefit of samplepgo, and it gets even better when comes to iterative compilation: more iteration will introduce richer context, and in later iterations, the real inline pass will be a noop, and samplepgo will perform perfect top-down inlining to capture all possible contexts.
> > > 
> > > Please let me know if you can think of better ways to force importing functions by simply looking at the profile. One thing I can think of is to pass the profile to thin_link, but it seems to add much complexity.
> > > 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.
> > 
> > In the case Dehao gave, which is foo_in_a_cc()->bar_in_b_cc()->baz_in_c_cc(), we presumably already have an edge in the call graph from foo_in_a_cc to bar_in_b_cc, this is just forcing that edge to be "hot" to force the import. The added edge that wouldn't have been there before would be foo_in_a_cc to baz_in_c_cc, and there we are adding a direct edge for what is a transitive dependence. I don't think this should cause any issues, since the other things we use the edges for are things like liveness and internalization, which aren't changed by adding a direct edge for the transitive dependence.
> > we are adding a direct edge for what is a transitive dependence. 
> 
> Right.
> 
> > I don't think this should cause any issues, since the other things we use the edges for are things like liveness and internalization, which aren't changed by adding a direct edge for the transitive dependence.
> 
> I don't see any issue *right now*, but that still means that our call-graph is not longer accurate, and that makes me worried about long-term consequences and what kind of analysis or heuristic could be affected by this. I'm concerned about things like that where we break some invariant of a data structure or a component and while it seems fine on the moment, it may bite in the future.
> 
> That said the alternative (adding an explicit handling for this) seems overkill at this point, so that's fine with me.
> 
Agree that we generally want the call graph to be accurate. I think there shouldn't ever be a correctness issue though when the call graph is more "conservative" as it will be here, with the additional edges. Note that in some sense the edges added for indirect calls can be conservative too - we may not actually call those same functions if the input changes.

https://reviews.llvm.org/D30053