Separate the logic when handling indirect calls in SamplePGO ThinLTO compile phase and other phases.

[danielcdh]

Summary

In SamplePGO ThinLTO compile phase, we will not invoke ICP as it may introduce confusion to the 2nd annotation. This patch extracted that logic and makes it clearer before profile annotation. In the mean time, we need to make function importing process both inlined callsites as well as not promoted indirect callsites.

[danielcdh]

refactor

[tejohnson]

Is this patch NFC? It looks like there is some new handling for indirect call targets in findImportedFunctions that makes it non-NFC. Could you split this into an NFC refactoring patch and a separate non-NFC patch with the enhancements if so? I think that would be clearer and easier to review.

include/llvm/ProfileData/SampleProf.h

342

Needs comment.

include/llvm/Transforms/SampleProfile.h

29

How about IsThinLTOPreLink for consistency with Phase? (Here and in other classes)

lib/Transforms/IPO/SampleProfile.cpp

89

Similar, let's use PreLink instead of Compile, to be consistent with terminology introduced for pass pipeline. After looking at how this is used, it does something different than what I originally thought. I first read this as whether to invoke this pass during the ThinLTO compile/pre-link, but it is actually forcing simulation of being in that phase. So maybe better to have something like "SampleProfileForceThinLTOPreLink" or similar.

I see this is used when invoked via opt. Can the phase be forced instead by using "-passes='thinlto-pre-link<O2>" (see test/Other/new-pm-thinlto-defaults.ll)?

[danielcdh]

In D38094#885238, @tejohnson wrote:

Is this patch NFC? It looks like there is some new handling for indirect call targets in findImportedFunctions that makes it non-NFC. Could you split this into an NFC refactoring patch and a separate non-NFC patch with the enhancements if so? I think that would be clearer and easier to review.

Submitted the NFC change in r314601 and rebased the patch

[danielcdh]

rebase and address Teresa's comments

[tejohnson]

include/llvm/ProfileData/SampleProf.h

343

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 (?)

lib/Transforms/IPO/SampleProfile.cpp

766

Suggest reversing the sense of this condition and moving the shorter pre link case up here.

[danielcdh]

update

[tejohnson]

lgtm

include/llvm/ProfileData/SampleProf.h

public:
   /// \p Threshold, add its corresponding GUID to \p S.
   void findImportedFunctions(DenseSet<GlobalValue::GUID> &S, const Module *M,
                              uint64_t Threshold) const {
     if (TotalSamples <= Threshold)
       return;
     Function *F = M->getFunction(Name);
     if (!F || !F->getSubprogram())
       S.insert(Function::getGUID(Name));
+    // 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.
+    for (const auto &BS : BodySamples)
+      for (const auto &TS : BS.second.getCallTargets())
+        if (TS.getValue() > Threshold) {
+          Function *Callee = M->getFunction(TS.getKey());
+          if (!Callee || !Callee->getSubprogram())
+            S.insert(Function::getGUID(TS.getKey()));
+        }
     for (auto CS : CallsiteSamples)
       for (const auto &NameFS : CS.second)
         NameFS.second.findImportedFunctions(S, M, Threshold);
   }
 
   /// Set the name of the function.
   void setName(StringRef FunctionName) { Name = FunctionName; }
 

include/llvm/Transforms/SampleProfile.h

 #include "llvm/IR/PassManager.h"
 
 namespace llvm {
 
 /// The sample profiler data loader pass.
 class SampleProfileLoaderPass : public PassInfoMixin<SampleProfileLoaderPass> {
 public:
   PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM);
-  SampleProfileLoaderPass(std::string File = "") : ProfileFileName(File) {}
+  SampleProfileLoaderPass(std::string File = "", bool IsThinLTOPreLink = false)
+      : ProfileFileName(File), IsThinLTOPreLink(IsThinLTOPreLink) {}
 
 private:
   std::string ProfileFileName;
+  bool IsThinLTOPreLink;
 };
 
 } // End llvm namespace
 #endif

lib/Passes/PassBuilder.cpp

PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   if (PGOOpt && !PGOOpt->SampleProfileFile.empty() &&
       Phase == ThinLTOPhase::PostLink)
     EarlyFPM.addPass(InstCombinePass());
   MPM.addPass(createModuleToFunctionPassAdaptor(std::move(EarlyFPM)));
 
   if (PGOOpt && !PGOOpt->SampleProfileFile.empty()) {
     // Annotate sample profile right after early FPM to ensure freshness of
     // the debug info.
-    MPM.addPass(SampleProfileLoaderPass(PGOOpt->SampleProfileFile));
+    MPM.addPass(SampleProfileLoaderPass(PGOOpt->SampleProfileFile,
+                                        Phase == ThinLTOPhase::PreLink));
     // Do not invoke ICP in the ThinLTOPrelink phase as it makes it hard
     // for the profile annotation to be accurate in the ThinLTO backend.
     if (Phase != ThinLTOPhase::PreLink)
       // We perform early indirect call promotion here, before globalopt.
       // This is important for the ThinLTO backend phase because otherwise
       // imported available_externally functions look unreferenced and are
       // removed.
       MPM.addPass(PGOIndirectCallPromotion(Phase == ThinLTOPhase::PostLink,

lib/Transforms/IPO/SampleProfile.cpp

 static cl::opt<unsigned> SampleProfileSampleCoverage(
     "sample-profile-check-sample-coverage", cl::init(0), cl::value_desc("N"),
     cl::desc("Emit a warning if less than N% of samples in the input profile "
              "are matched to the IR."));
 static cl::opt<double> SampleProfileHotThreshold(
     "sample-profile-inline-hot-threshold", cl::init(0.1), cl::value_desc("N"),
     cl::desc("Inlined functions that account for more than N% of all samples "
              "collected in the parent function, will be inlined again."));
 
 namespace {
 typedef DenseMap<const BasicBlock *, uint64_t> BlockWeightMap;
 typedef DenseMap<const BasicBlock *, const BasicBlock *> EquivalenceClassMap;
 typedef std::pair<const BasicBlock *, const BasicBlock *> Edge;
 typedef DenseMap<Edge, uint64_t> EdgeWeightMap;
 typedef DenseMap<const BasicBlock *, SmallVector<const BasicBlock *, 8>>
     BlockEdgeMap;
 

 /// \brief Sample profile pass.
 ///
 /// This pass reads profile data from the file specified by
 /// -sample-profile-file and annotates every affected function with the
 /// profile information found in that file.
 class SampleProfileLoader {
 public:
   SampleProfileLoader(
-      StringRef Name,
+      StringRef Name, bool IsThinLTOPreLink,
       std::function<AssumptionCache &(Function &)> GetAssumptionCache,
       std::function<TargetTransformInfo &(Function &)> GetTargetTransformInfo)
       : DT(nullptr), PDT(nullptr), LI(nullptr), GetAC(GetAssumptionCache),
         GetTTI(GetTargetTransformInfo), Reader(), Samples(nullptr),
-        Filename(Name), ProfileIsValid(false), TotalCollectedSamples(0),
-        ORE(nullptr) {}
+        Filename(Name), ProfileIsValid(false),
+        IsThinLTOPreLink(IsThinLTOPreLink),
+        TotalCollectedSamples(0), ORE(nullptr) {}
 
   bool doInitialization(Module &M);
   bool runOnModule(Module &M, ModuleAnalysisManager *AM);
 
   void dump() { Reader->dump(); }
 
 protected:
   bool runOnFunction(Function &F, ModuleAnalysisManager *AM);
protected:
   FunctionSamples *Samples;
 
   /// \brief Name of the profile file to load.
   std::string Filename;
 
   /// \brief Flag indicating whether the profile input loaded successfully.
   bool ProfileIsValid;
 
+  /// \brief Flag indicating if the pass is invoked in ThinLTO compile phase.
+  ///
+  /// In this phase, in annotation, we should not promote indirect calls.
+  /// Instead, we will mark GUIDs that needs to be annotated to the function.
+  bool IsThinLTOPreLink;
+
   /// \brief Total number of samples collected in this profile.
   ///
   /// This is the sum of all the samples collected in all the functions executed
   /// at runtime.
   uint64_t TotalCollectedSamples;
 
   /// \brief Optimization Remark Emitter used to emit diagnostic remarks.
   OptimizationRemarkEmitter *ORE;
 };
 
 class SampleProfileLoaderLegacyPass : public ModulePass {
 public:
   // Class identification, replacement for typeinfo
   static char ID;
 
-  SampleProfileLoaderLegacyPass(StringRef Name = SampleProfileFile)
-      : ModulePass(ID), SampleLoader(Name,
+  SampleProfileLoaderLegacyPass(StringRef Name = SampleProfileFile,
+                                bool IsThinLTOPreLink = false)
+      : ModulePass(ID), SampleLoader(Name, IsThinLTOPreLink,
                                      [&](Function &F) -> AssumptionCache & {
                                        return ACT->getAssumptionCache(F);
                                      },
                                      [&](Function &F) -> TargetTransformInfo & {
                                        return TTIWP->getTTI(F);
                                      }),
         ACT(nullptr), TTIWP(nullptr) {
     initializeSampleProfileLoaderLegacyPassPass(
for (auto I : CIS) {
       Function *CalledFunction = CallSite(I).getCalledFunction();
       // Do not inline recursive calls.
       if (CalledFunction == &F)
         continue;
       if (CallSite(I).isIndirectCall()) {
         if (PromotedInsns.count(I))
           continue;
         for (const auto *FS : findIndirectCallFunctionSamples(*I)) {
+          if (!IsThinLTOPreLink) {
             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;
             } else {
+              DEBUG(dbgs()
+                    << "\nFailed to promote indirect call to "
+                    << CalleeFunctionName << " because " << Reason << "\n");
+              continue;
+            }
+          } else {
             FS->findImportedFunctions(ImportGUIDs, F.getParent(),
                                       Samples->getTotalSamples() *
                                           SampleProfileHotThreshold / 100);
           }
         }
       } else if (CalledFunction && CalledFunction->getSubprogram() &&
                  !CalledFunction->isDeclaration()) {
         if (inlineCallInstruction(I))
           LocalChanged = true;
-      } else {
+      } else if (IsThinLTOPreLink) {
         findCalleeFunctionSamples(*I)->findImportedFunctions(
             ImportGUIDs, F.getParent(),
             Samples->getTotalSamples() * SampleProfileHotThreshold / 100);
       }
     }
     if (LocalChanged) {
       Changed = true;
     } else {
PreservedAnalyses SampleProfileLoaderPass::run(Module &M,
 
   auto GetAssumptionCache = [&](Function &F) -> AssumptionCache & {
     return FAM.getResult<AssumptionAnalysis>(F);
   };
   auto GetTTI = [&](Function &F) -> TargetTransformInfo & {
     return FAM.getResult<TargetIRAnalysis>(F);
   };
 
-  SampleProfileLoader SampleLoader(ProfileFileName.empty() ? SampleProfileFile
-                                                           : ProfileFileName,
-                                   GetAssumptionCache, GetTTI);
+  SampleProfileLoader SampleLoader(
+      ProfileFileName.empty() ? SampleProfileFile : ProfileFileName,
+      IsThinLTOPreLink, GetAssumptionCache, GetTTI);
 
   SampleLoader.doInitialization(M);
 
   if (!SampleLoader.runOnModule(M, &AM))
     return PreservedAnalyses::all();
 
   return PreservedAnalyses::none();
 }

test/Transforms/SampleProfile/Inputs/import.prof

 test:10000:0
  3: foo:1000
   3: bar:200
    4: baz:10
  4: foo1:1000
   1: 1000
  4: foo2:1000
-  1: 1000
+  1: 1000 foo3:1000

test/Transforms/SampleProfile/import.ll

-; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/import.prof -S | FileCheck %s
+; RUN: opt < %s -passes='thinlto-pre-link<O2>' -pgo-kind=new-pm-pgo-sample-use-pipeline -profile-file=%S/Inputs/import.prof -S | FileCheck %s
 
 ; Tests whether the functions in the inline stack are added to the
 ; function_entry_count metadata.
 
 declare void @foo()
 
 define void @test(void ()*) !dbg !7 {
   %2 = alloca void ()*
   store void ()* %0, void ()** %2
   %3 = load void ()*, void ()** %2
   ; CHECK: call {{.*}}, !prof ![[PROF:[0-9]+]]
   call void @foo(), !dbg !18
   call void %3(), !dbg !19
   ret void
 }
 
-; GUIDs of foo, bar, foo1 and foo2 should be included in the metadata to make
-; sure hot inline stacks are imported.
-; CHECK: !{!"function_entry_count", i64 1, i64 2494702099028631698, i64 6699318081062747564, i64 7682762345278052905, i64 -2012135647395072713}
+; GUIDs of foo, bar, foo1, foo2 and foo3 should be included in the metadata to
+; make sure hot inline stacks are imported.
+; CHECK: !{!"function_entry_count", i64 1, i64 2494702099028631698, i64 6699318081062747564, i64 7682762345278052905,  i64 -7908226060800700466, i64 -2012135647395072713}
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!8, !9}
 !llvm.ident = !{!10}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, producer: "clang version 3.5 ", isOptimized: false, emissionKind: NoDebug, file: !1, enums: !2, retainedTypes: !2, globals: !2, imports: !2)
 !1 = !DIFile(filename: "calls.cc", directory: ".")
 !2 = !{}