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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D30053#679300, @mehdi_amini wrote:

> In https://reviews.llvm.org/D30053#679264, @davidxl wrote:
>
> > Is it better to introduce a new meta data for this, e.g. MD_inline_instance_imports ?   Can this information be directly communicated to function importer instead of relying on modifying Callgraph/profile data?
>
>
> We need to expose this to the thin-link for the distributed mode, otherwise the dependent files the function importer would want to import from may not be available. So the summary-based importing analysis will need to be aware of this. Right now the solution is to trick it like we do for ICP by adding "fake" edges. The difference is that for ICP we just make indirect edges direct, but we don't really cheat on the graph structure otherwise. Here we'd create "short-circuit" edges (adding an edge foo->bar when in fact the reality is foo->baz->bar).


It isn't just for the distributed mode - the Thin Link needs to know this information so that it can appropriately mark references as exported . We need to make all importing decisions in the Thin Link not the ThinLTO backends, so it needs to be in the summary in some form. Adding the direct edges for the transitive edges seemed like the cleanest way to do this as it doesn't require any changes to the function importing decisions during the thin link, rather than adding a new mechanism.



================
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.
----------------
Think this needs more explanation with a small example like the one you gave in the review discussion.


================
Comment at: include/llvm/IR/Function.h:212
+  /// pgo data. \p Imports points to a set of GUIDs that needs to be imported
+  /// by the function.
+  void setEntryCount(uint64_t Count,
----------------
add something like "for sample PGO, to enable the same inlines as the profiled optimized binary".


================
Comment at: include/llvm/IR/Function.h:222
 
+  /// Returns the set of GUIDs that needs to be imported to the function.
+  DenseSet<GlobalValue::GUID> getImportGUIDs() const;
----------------
ditto


================
Comment at: include/llvm/IR/MDBuilder.h:70
+  MDNode *createFunctionEntryCount(uint64_t Count,
+                                   const DenseSet<GlobalValue::GUID> *Imports);
 
----------------
Document the new parameter.


================
Comment at: include/llvm/ProfileData/SampleProf.h:309
+  /// \p Threshold, add its corresponding GUID to \p S.
+  void importAllFunctions(DenseSet<GlobalValue::GUID> &S, const Module *M,
+                          uint64_t Threshold) const {
----------------
Since this isn't actually doing any importing, I think it would be clearer to rename to something like "findImportedFunctions" (i.e. it is identifying functions that were presumably imported and inlined in the profiled binary).


================
Comment at: lib/Analysis/ModuleSummaryAnalysis.cpp:186
 
+  for (auto &I : F.getImportGUIDs())
+    CallGraphEdges[I].updateHotness(CalleeInfo::HotnessType::Hot);
----------------
Needs comment


================
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:
> > > > > > 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.


================
Comment at: lib/IR/Function.cpp:1305
+  MDNode *MD = getMetadata(LLVMContext::MD_prof);
+  if (MD && MD->getOperand(0))
+    if (MDString *MDS = dyn_cast<MDString>(MD->getOperand(0)))
----------------
When would we have the metadata but not the operand?


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:610
+bool SampleProfileLoader::inlineHotFunctions(
+    Function &F, DenseSet<GlobalValue::GUID> &ImportGUIDs) {
   DenseSet<Instruction *> PromotedInsns;
----------------
Document new parameter


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:1285
+    // Add an entry count to the function using the samples gathered
+    // at the function entry.
+    F.setEntryCount(Samples->getHeadSamples() + 1, &ImportGUIDs);
----------------
note that this is also recording the GUIDs that need to be imported for the IR to match


https://reviews.llvm.org/D30053