[PATCH] D38094: Separate the logic when handling indirect calls in SamplePGO ThinLTO compile phase and other phases.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: include/llvm/ProfileData/SampleProf.h:343
+    // Import hot callsites that are not inlined in the profile.
+    // We need it because if we may not be able to import and inline FS to
+    // expose the callsite as IR in ThinLTO prelink phase.
----------------
What is "FS"? Also, the sentence is unclear, partly because there is an "if" with no corresponding "then".  Is this handling indirect or direct calls (CallTargets is not documented - could you add a doxygen description to it)? From the comments it sounds like this is looking for additional hot targets that we didn't import/inline in the profiled binary, but might want to now with additional profile data (?)


================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:766
         for (const auto *FS : findIndirectCallFunctionSamples(*I)) {
-          auto CalleeFunctionName = FS->getName();
-          // If it is a recursive call, we do not inline it as it could bloat
-          // the code exponentially. There is way to better handle this, e.g.
-          // clone the caller first, and inline the cloned caller if it is
-          // recursive. As llvm does not inline recursive calls, we will
-          // simply ignore it instead of handling it explicitly.
-          if (CalleeFunctionName == F.getName())
-            continue;
-
-          const char *Reason = "Callee function not available";
-          auto R = SymbolMap.find(CalleeFunctionName);
-          if (R != SymbolMap.end() && R->getValue() &&
-              !R->getValue()->isDeclaration() &&
-              R->getValue()->getSubprogram() &&
-              isLegalToPromote(I, R->getValue(), &Reason)) {
-            // The indirect target was promoted and inlined in the profile,
-            // as a result, we do not have profile info for the branch
-            // probability. We set the probability to 80% taken to indicate
-            // that the static call is likely taken.
-            Instruction *DI = dyn_cast<Instruction>(
-                promoteIndirectCall(I, R->getValue(), 80, 100, false, ORE)
-                    ->stripPointerCasts());
-            PromotedInsns.insert(I);
-            // If profile mismatches, we should not attempt to inline DI.
-            if ((isa<CallInst>(DI) || isa<InvokeInst>(DI)) &&
-                inlineCallInstruction(DI))
-              LocalChanged = true;
+          if (!IsThinLTOPreLink) {
+            auto CalleeFunctionName = FS->getName();
----------------
Suggest reversing the sense of this condition and moving the shorter pre link case up here.


https://reviews.llvm.org/D38094