Invoke GetInlineCost for legality check before inline functions in SampleProfileLoader.

[danielcdh]

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.

[eraman]

lib/Transforms/IPO/SampleProfile.cpp

89

Do you really need an option for this? All you want is to pick a large enough number such that the cost never exceeds the threshold.

760

This needs detailed comments. State that you want to check if there is anything in the reachable portion of the callee at this callsite that makes this inlining potentially illegal and that's why you use a large threshold (large enough to return true in ~all cases except when it is potentially incorrect)

761

Differentiate the InlineCost::isNever and the case where cost exceeds threshold. It is useful to log the latter case for debugging.

[danielcdh]

update

[eraman]

LGTM

[eraman]

Discussed this offline with Dehao. Instead of using an arbitrary threshold of 100000, it is better to piggyback on the the option in InlineCost to compute cost analysis in full. That way, if the analysis returns NeverInlineCost, Sample loader shouldn't inline. Anything else (doesn't matter if the cost >= threshold), it can do the inlining. I will refactor InlineCost.cpp to enable the feature to be used without requiring an option and Dehao will base this patch on top of that.

[danielcdh]

rebase

[eraman]

LGTM

lib/Transforms/IPO/SampleProfile.cpp

763–778

Nit: I would use Params instead (or IP.)

768

Extend the comment to say that the actual threshold doesn't matter since you only check for isNever.

[danielcdh]

update

[vitalybuka]

There is a uninitialized value, so I've reverted the patch with r313230 to fix bots.
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/7858/steps/check-llvm%20msan/logs/stdio

[danielcdh]

fix the msan uninitialized memory access error.

lib/Transforms/IPO/SampleProfile.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/SampleProfile.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #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"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/Instructions.h"

 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;
 

 ///
 /// 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,
-      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);
 
   void dump() { Reader->dump(); }
 
 protected:
   bool runOnFunction(Function &F, ModuleAnalysisManager *AM);
protected:
   StringMap<Function *> SymbolMap;
 
   /// \brief Dominance, post-dominance and loop information.
   std::unique_ptr<DominatorTree> DT;
   std::unique_ptr<PostDomTreeBase<BasicBlock>> PDT;
   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;
 
   /// \brief Successors for each basic block in the CFG.
   BlockEdgeMap Successors;
 
   SampleCoverageTracker CoverageTracker;

 public:
   // Class identification, replacement for typeinfo
   static char ID;
 
   SampleProfileLoaderLegacyPass(StringRef Name = SampleProfileFile)
       : 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());
   }
 
   void dump() { SampleLoader.dump(); }
 
   bool doInitialization(Module &M) override {
     return SampleLoader.doInitialization(M);
   }
   StringRef getPassName() const override { return "Sample profile pass"; }
   bool runOnModule(Module &M) override;
 
   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.
 ///
 /// Functions that were inlined in the original binary will be represented
 /// in the inline stack in the sample profile. If the profile shows that
 /// the original inline decision was "good" (i.e., the callsite is executed
 /// frequently), then we will recreate the inline decision and apply the
for (auto I : CIS) {
           }
         }
       if (!CalledFunction || !CalledFunction->getSubprogram()) {
         findCalleeFunctionSamples(*I)->findImportedFunctions(
             ImportGUIDs, F.getParent(),
             Samples->getTotalSamples() * SampleProfileHotThreshold / 100);
         continue;
       }
+      CallSite CS(DI);
       DebugLoc DLoc = I->getDebugLoc();
       BasicBlock *BB = I->getParent();
-      if (InlineFunction(CallSite(DI), IFI)) {
+      InlineParams Params = getInlineParams();
+      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)
                   << "inlined hot callee '"
                   << ore::NV("Callee", CalledFunction) << "' into '"
                   << ore::NV("Caller", &F) << "'");
       }
     }
bool SampleProfileLoader::emitAnnotations(Function &F) {
   }
   return Changed;
 }
 
 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)
 
 bool SampleProfileLoader::doInitialization(Module &M) {
   auto &Ctx = M.getContext();
   auto ReaderOrErr = SampleProfileReader::create(Filename, Ctx);
   if (std::error_code EC = ReaderOrErr.getError()) {
     std::string Msg = "Could not open profile: " + EC.message();
for (auto &F : M)
     }
   if (M.getProfileSummary() == nullptr)
     M.setProfileSummary(Reader->getSummary().getMD(M.getContext()));
   return retval;
 }
 
 bool SampleProfileLoaderLegacyPass::runOnModule(Module &M) {
   ACT = &getAnalysis<AssumptionCacheTracker>();
+  TTIWP = &getAnalysis<TargetTransformInfoWrapperPass>();
   return SampleLoader.runOnModule(M, nullptr);
 }
 
 bool SampleProfileLoader::runOnFunction(Function &F, ModuleAnalysisManager *AM) {
   F.setEntryCount(0);
   std::unique_ptr<OptimizationRemarkEmitter> OwnedORE;
   if (AM) {
     auto &FAM =

 PreservedAnalyses SampleProfileLoaderPass::run(Module &M,
                                                ModuleAnalysisManager &AM) {
   FunctionAnalysisManager &FAM =
       AM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
 
   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);
 
   if (!SampleLoader.runOnModule(M, &AM))
     return PreservedAnalyses::all();
 
   return PreservedAnalyses::none();
 }

test/Transforms/SampleProfile/Inputs/einline.prof

 _Z3foov:200:100
  1: _Z3barv:0
+ 2: no_inline:100
  3: _Z3barv:100
 recursive:200:100
  1: recursive:100
  2: recursive:100

test/Transforms/SampleProfile/early-inline.ll

 ; RUN: opt < %s -instcombine -sample-profile -sample-profile-file=%S/Inputs/einline.prof -S | FileCheck %s
 
 ; Checks if both call and invoke can be inlined early if their inlined
 ; instances are hot in profile.
 
 target triple = "x86_64-unknown-linux-gnu"
 
 @_ZTIi = external constant i8*
 
 ; Function Attrs: uwtable
 define void @_Z3foov() #0 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) !dbg !6 {
   %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
   invoke void @_ZL3barv()
           to label %4 unwind label %5, !dbg !10
 
 ; <label>:4:
   ret void

 ; CHECK: call void @recursive
 ; CHECK-NOT: call void @recursive
 ; CHECK: ret
   call void @recursive(), !dbg !14
   call void @recursive(), !dbg !15
   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}
 
 !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1)
 !1 = !DIFile(filename: "a", directory: "b/")
 !3 = !{i32 2, !"Dwarf Version", i32 4}
 !4 = !{i32 2, !"Debug Info Version", i32 3}
 !6 = distinct !DISubprogram(linkageName: "_Z3foov", scope: !1, file: !1, line: 5, scopeLine: 5, unit: !0)
 !9 = !DILocation(line: 6, column: 3, scope: !6)
 !10 = !DILocation(line: 8, column: 5, scope: !11)
 !11 = distinct !DILexicalBlock(scope: !6, file: !1, line: 7, column: 7)
 !12 = distinct !DISubprogram(linkageName: "_ZL3barv", scope: !1, file: !1, line: 20, scopeLine: 20, unit: !0)
 !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)