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

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.

Reviewers: tejohnson

Reviewed By: tejohnson

Subscribers: sanjoy, mehdi_amini, llvm-commits, inglorion

Differential Revision: https://reviews.llvm.org/D38094

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@314619 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/llvm/ProfileData/SampleProf.h b/include/llvm/ProfileData/SampleProf.h
index 7fc258831be8831628b01e9a575a2e5523152c25..3e17df1480798de5c5328a3fcf8ed9468845b80a 100644 (file)
--- a/include/llvm/ProfileData/SampleProf.h
+++ b/include/llvm/ProfileData/SampleProf.h
@@ -331,7 +331,8 @@ public:
 
   /// Recursively traverses all children, if the corresponding function is
   /// not defined in module \p M, and its total sample is no less than
-  /// \p Threshold, add its corresponding GUID to \p S.
+  /// \p Threshold, add its corresponding GUID to \p S. Also traverse the
+  /// BodySamples to add hot CallTarget's GUID to \p S.
   void findImportedFunctions(DenseSet<GlobalValue::GUID> &S, const Module *M,
                              uint64_t Threshold) const {
     if (TotalSamples <= Threshold)
@@ -339,6 +340,15 @@ public:
     Function *F = M->getFunction(Name);
     if (!F || !F->getSubprogram())
       S.insert(Function::getGUID(Name));
+    // Import hot CallTargets, which may not be available in IR because full
+    // profile annotation cannot be done until backend compilation in ThinLTO.
+    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);

diff --git a/include/llvm/Transforms/SampleProfile.h b/include/llvm/Transforms/SampleProfile.h
index c984fe74ba939c1ab8ef020ca2f284b45e259120..8f9707835651f38c05a7143f165dfcbb855b1502 100644 (file)
--- a/include/llvm/Transforms/SampleProfile.h
+++ b/include/llvm/Transforms/SampleProfile.h
@@ -21,10 +21,12 @@ namespace llvm {
 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

diff --git a/lib/Passes/PassBuilder.cpp b/lib/Passes/PassBuilder.cpp
index c277b5b14e750f233e3b54885c90e7f6c8b40225..c9cc0c5ae3811509d6f9087ed504b08aea28a51d 100644 (file)
--- a/lib/Passes/PassBuilder.cpp
+++ b/lib/Passes/PassBuilder.cpp
@@ -555,7 +555,8 @@ PassBuilder::buildModuleSimplificationPipeline(OptimizationLevel Level,
   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)

diff --git a/lib/Transforms/IPO/SampleProfile.cpp b/lib/Transforms/IPO/SampleProfile.cpp
index 93e697c9a7eb64e345ba7d9d222b441bec401a49..7dce904ff157a032e1fbadd2a2895f716a0f527a 100644 (file)
--- a/lib/Transforms/IPO/SampleProfile.cpp
+++ b/lib/Transforms/IPO/SampleProfile.cpp
@@ -149,13 +149,14 @@ private:
 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);
@@ -252,6 +253,12 @@ protected:
   /// \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
@@ -267,8 +274,9 @@ 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);
                                      },
@@ -755,6 +763,12 @@ bool SampleProfileLoader::inlineHotFunctions(
         if (PromotedInsns.count(I))
           continue;
         for (const auto *FS : findIndirectCallFunctionSamples(*I)) {
+          if (IsThinLTOPreLink) {
+            FS->findImportedFunctions(ImportGUIDs, F.getParent(),
+                                      Samples->getTotalSamples() *
+                                          SampleProfileHotThreshold / 100);
+            continue;
+          }
           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.
@@ -783,16 +797,16 @@ bool SampleProfileLoader::inlineHotFunctions(
                 inlineCallInstruction(DI))
               LocalChanged = true;
           } else {
-            FS->findImportedFunctions(ImportGUIDs, F.getParent(),
-                                      Samples->getTotalSamples() *
-                                          SampleProfileHotThreshold / 100);
+            DEBUG(dbgs()
+                  << "\nFailed to promote indirect call to "
+                  << CalleeFunctionName << " because " << Reason << "\n");
           }
         }
       } 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);
@@ -1558,9 +1572,9 @@ PreservedAnalyses SampleProfileLoaderPass::run(Module &M,
     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);
 

diff --git a/test/Transforms/SampleProfile/Inputs/import.prof b/test/Transforms/SampleProfile/Inputs/import.prof
index aae072ac191fc12938c125527caa510bae958e78..e09ee6bf06057696c2d044d50e52475b9655bd29 100644 (file)
--- a/test/Transforms/SampleProfile/Inputs/import.prof
+++ b/test/Transforms/SampleProfile/Inputs/import.prof
@@ -5,4 +5,4 @@ test:10000:0
  4: foo1:1000
   1: 1000
  4: foo2:1000
-  1: 1000
+  1: 1000 foo3:1000

diff --git a/test/Transforms/SampleProfile/import.ll b/test/Transforms/SampleProfile/import.ll
index ad9c2d55aa904ecf6a24a15f2b06187edff32353..8cc2338a04895b4109389cd9ae94d75aa0f7a091 100644 (file)
--- a/test/Transforms/SampleProfile/import.ll
+++ b/test/Transforms/SampleProfile/import.ll
@@ -1,4 +1,4 @@
-; 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.
@@ -15,9 +15,9 @@ define void @test(void ()*) !dbg !7 {
   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}