[PATCH] D39480: Include GUIDs from the same module when computing GUIDs that needs to be imported.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D39480#912335, @danielcdh wrote:

> In https://reviews.llvm.org/D39480#912334, @tejohnson wrote:
>
> > In https://reviews.llvm.org/D39480#912321, @danielcdh wrote:
> >
> > > In https://reviews.llvm.org/D39480#912309, @tejohnson wrote:
> > >
> > > > Does VP metadata get synthesized from the SamplePGO profile? The original bug report had some of the targets on VP metadata attached to the indirect call, but was missing others. Why not include all the targets on that VP metadata rather than marking them for import which seems more hacky.
> > >
> > >
> > > Because we need to handle the indirect call chain too. E.g. foo() indirectly calls bar(), which indirectly calls baz(). Assuming these 2 calls are ICPed and inlined in the profiling binary, when processing foo's indirect callsite, marking bar's GUID in the VP is not enough, but adding baz's GUID to foo's callsite does not make sense as baz is not its call target. That's why we need to keep the transitive closure as function level metadata.
> >
> >
> > But the case here is when they are in fact in the same module. I'm assuming the situation you describe here already works when bar and baz are in different modules than foo. In this case we have foo and bar in the same module, and were missing the VP metadata showing that foo calls bar indirectly. If baz is also in the same module then the same situation applies to the callsite in bar. If it is in a different module then wouldn't the existing logic handle it? I.e. it would be on bar's function entry metadata?
>
>
> If baz is in the same module as bar and foo, then we need to have baz in foo's entry metadata. For the callsite in bar (standalone copy), it is possible that bar becomes cold after inlined into foo thus there is no profile to indicate bar has an indirect call to baz.
>
> If baz is in another module, then in the current implementation (without this patch), it will be added in foo's entry metadata.


After discussing offline with Dehao, I understand the issue:

- just an intra-module indirect call could be handled by doing the promotion/inlining during the compile step (or annotating it on the VP metadata attached to the indirect call)
- the issue is when you have a call out of the module, that then does an indirect call back into the module, where the whole call chain was promoted/inlined in the profiled binary: A.c:foo() ->(direct or indirect)-> B.c:bar() ->(indirect)-> A.c:baz()

If the indirect calls along this chain were promoted and then inlined into foo in the profiled binary, we cannot do the promotion/inlining of the call back to A.c:baz() during the compile step since it is intra-module, but we need to be able to annotate the call somewhere. And apparently we can't do this on the VP metadata for the indirect call in B.c:bar() since it was inlined into foo() in the profiled binary (Dehao - is that correct?).

In that case, I have a couple suggestions for the comments and test below.



================
Comment at: include/llvm/ProfileData/SampleProf.h:359
   /// BodySamples to add hot CallTarget's GUID to \p S.
   void findImportedFunctions(DenseSet<GlobalValue::GUID> &S, const Module *M,
                              uint64_t Threshold) const {
----------------
Function name and comment need updating, since we aren't necessarily importing, but need to register the promoted indirect calls for liveness analysis.


================
Comment at: test/Transforms/SampleProfile/import.ll:8
 
+define void @foo_available() !dbg !11 {
+  ret void
----------------
Suggest making this a different test with a different name since we aren't testing importing, but rather liveness analysis. In that case it would also be good to add a check for the situation described earlier as well.
 A.c:foo() ->(direct or indirect)-> B.c:bar() ->(indirect)-> A.c:baz()


https://reviews.llvm.org/D39480