Invoke GetInlineCost for legality check before inline functions in SampleProfileLoader.

Summary: SampleProfileLoader inlines hot functions if it is inlined in the profiled binary. However, the inline needs to be guarded by legality check, otherwise it could lead to correctness issues.

Reviewers: eraman, davidxl

Reviewed By: eraman

Subscribers: sanjoy, llvm-commits

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

llvm-svn: 313195

3 files changed, 50 insertions, 6 deletions

diff --git a/llvm/lib/Transforms/IPO/SampleProfile.cpp b/llvm/lib/Transforms/IPO/SampleProfile.cpp
index f13ddee3699..a9d3ef8d32a 100644
--- a/llvm/lib/Transforms/IPO/SampleProfile.cpp
+++ b/llvm/lib/Transforms/IPO/SampleProfile.cpp
@@ -28,9 +28,11 @@
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Analysis/AssumptionCache.h"
+#include "llvm/Analysis/InlineCost.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/Analysis/OptimizationDiagnosticInfo.h"
 #include "llvm/Analysis/PostDominators.h"
+#include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DebugInfo.h"
 #include "llvm/IR/DiagnosticInfo.h"
@@ -148,10 +150,12 @@ class SampleProfileLoader {
 public:
   SampleProfileLoader(
       StringRef Name,
-      std::function<AssumptionCache &(Function &)> GetAssumptionCache)
+      std::function<AssumptionCache &(Function &)> GetAssumptionCache,
+      std::function<TargetTransformInfo &(Function &)> GetTargetTransformInfo)
       : DT(nullptr), PDT(nullptr), LI(nullptr), GetAC(GetAssumptionCache),
-        Reader(), Samples(nullptr), Filename(Name), ProfileIsValid(false),
-        TotalCollectedSamples(0), ORE(nullptr) {}
+        GetTTI(GetTargetTransformInfo), Reader(), Samples(nullptr),
+        Filename(Name), ProfileIsValid(false), TotalCollectedSamples(0),
+        ORE(nullptr) {}
 
   bool doInitialization(Module &M);
   bool runOnModule(Module &M, ModuleAnalysisManager *AM);
@@ -225,6 +229,7 @@ protected:
   std::unique_ptr<LoopInfo> LI;
 
   std::function<AssumptionCache &(Function &)> GetAC;
+  std::function<TargetTransformInfo &(Function &)> GetTTI;
 
   /// \brief Predecessors for each basic block in the CFG.
   BlockEdgeMap Predecessors;
@@ -265,8 +270,11 @@ public:
       : ModulePass(ID), SampleLoader(Name,
                                      [&](Function &F) -> AssumptionCache & {
                                        return ACT->getAssumptionCache(F);
+                                     },
+                                     [&](Function &F) -> TargetTransformInfo & {
+                                       return TTIWP->getTTI(F);
                                      }),
-        ACT(nullptr) {
+        ACT(nullptr), TTIWP(nullptr) {
     initializeSampleProfileLoaderLegacyPassPass(
         *PassRegistry::getPassRegistry());
   }
@@ -281,11 +289,13 @@ public:
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.addRequired<AssumptionCacheTracker>();
+    AU.addRequired<TargetTransformInfoWrapperPass>();
   }
 
 private:
   SampleProfileLoader SampleLoader;
   AssumptionCacheTracker *ACT;
+  TargetTransformInfoWrapperPass *TTIWP;
 };
 
 /// Return true if the given callsite is hot wrt to its caller.
@@ -747,9 +757,25 @@ bool SampleProfileLoader::inlineHotFunctions(
             Samples->getTotalSamples() * SampleProfileHotThreshold / 100);
         continue;
       }
+      CallSite CS(DI);
       DebugLoc DLoc = I->getDebugLoc();
       BasicBlock *BB = I->getParent();
-      if (InlineFunction(CallSite(DI), IFI)) {
+      InlineParams Params;
+      Params.ComputeFullInlineCost = true;
+      // Checks if there is anything in the reachable portion of the callee at
+      // this callsite that makes this inlining potentially illegal. Need to
+      // set ComputeFullInlineCost, otherwise getInlineCost may return early
+      // when cost exceeds threshold without checking all IRs in the callee.
+      // The acutal cost does not matter because we only checks isNever() to
+      // see if it is legal to inline the callsite.
+      InlineCost Cost = getInlineCost(CS, Params, GetTTI(*CalledFunction), GetAC,
+                                      None, nullptr, nullptr);
+      if (Cost.isNever()) {
+        ORE->emit(OptimizationRemark(DEBUG_TYPE, "Not inline", DLoc, BB)
+                  << "incompatible inlining");
+        continue;
+      }
+      if (InlineFunction(CS, IFI)) {
         LocalChanged = true;
         // The call to InlineFunction erases DI, so we can't pass it here.
         ORE->emit(OptimizationRemark(DEBUG_TYPE, "HotInline", DLoc, BB)
@@ -1418,6 +1444,7 @@ char SampleProfileLoaderLegacyPass::ID = 0;
 INITIALIZE_PASS_BEGIN(SampleProfileLoaderLegacyPass, "sample-profile",
                       "Sample Profile loader", false, false)
 INITIALIZE_PASS_DEPENDENCY(AssumptionCacheTracker)
+INITIALIZE_PASS_DEPENDENCY(TargetTransformInfoWrapperPass)
 INITIALIZE_PASS_END(SampleProfileLoaderLegacyPass, "sample-profile",
                     "Sample Profile loader", false, false)
 
@@ -1483,6 +1510,7 @@ bool SampleProfileLoader::runOnModule(Module &M, ModuleAnalysisManager *AM) {
 
 bool SampleProfileLoaderLegacyPass::runOnModule(Module &M) {
   ACT = &getAnalysis<AssumptionCacheTracker>();
+  TTIWP = &getAnalysis<TargetTransformInfoWrapperPass>();
   return SampleLoader.runOnModule(M, nullptr);
 }
 
@@ -1512,10 +1540,13 @@ 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);
+                                   GetAssumptionCache, GetTTI);
 
   SampleLoader.doInitialization(M);
 
diff --git a/llvm/test/Transforms/SampleProfile/Inputs/einline.prof b/llvm/test/Transforms/SampleProfile/Inputs/einline.prof
index df0275b5005..624990b47ef 100644
--- a/llvm/test/Transforms/SampleProfile/Inputs/einline.prof
+++ b/llvm/test/Transforms/SampleProfile/Inputs/einline.prof
@@ -1,5 +1,6 @@
 _Z3foov:200:100
  1: _Z3barv:0
+ 2: no_inline:100
  3: _Z3barv:100
 recursive:200:100
  1: recursive:100
diff --git a/llvm/test/Transforms/SampleProfile/early-inline.ll b/llvm/test/Transforms/SampleProfile/early-inline.ll
index a240635b2b8..042713adc75 100644
--- a/llvm/test/Transforms/SampleProfile/early-inline.ll
+++ b/llvm/test/Transforms/SampleProfile/early-inline.ll
@@ -12,6 +12,8 @@ define void @_Z3foov() #0 personality i8* bitcast (i32 (...)* @__gxx_personality
   %1 = alloca i8*
   %2 = alloca i32
   %3 = alloca i32, align 4
+; CHECK: call void @no_inline
+  call void @no_inline(), !dbg !16
 ; CHECK-NOT: call
   call void @_ZL3barv(), !dbg !9
 ; CHECK-NOT: invoke
@@ -45,8 +47,16 @@ define void @recursive() !dbg !13 {
   ret void
 }
 
+; The callee has mismatch attributes to the caller, it should not be inlined
+define void @no_inline() #1 !dbg !17 {
+  ret void
+}
+
 declare i32 @__gxx_personality_v0(...)
 
+attributes #0 = {"target-features"="+sse4.1"}
+attributes #1 = {"target-features"="+sse4.2"}
+
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4}
 
@@ -62,3 +72,5 @@ declare i32 @__gxx_personality_v0(...)
 !13 = distinct !DISubprogram(linkageName: "recursive", scope: !1, file: !1, line: 20, scopeLine: 20, unit: !0)
 !14 = !DILocation(line: 21, column: 3, scope: !13)
 !15 = !DILocation(line: 22, column: 3, scope: !13)
+!16 = !DILocation(line: 7, column: 3, scope: !6)
+!17 = distinct !DISubprogram(linkageName: "no_inline", scope: !1, file: !1, line: 20, scopeLine: 20, unit: !0)