Build SymbolMap in SampleProfileLoader to help matchin function names with suffix.

[danielcdh]

Summary

If there is suffix added in the function name (e.g. module hash added by thinLTO), we will not be able to find a match in profile as the suffix does not exist in profile. This patch build a map from function name to Function *. The map includes the entry for the stripped function name so that inlineHotFunctions can find the corresponding function to promote/inline.

[danielcdh]

rebase

[davidxl]

lib/Transforms/IPO/SampleProfile.cpp

1439

Why erasing the existing entry?

[danielcdh]

add comment

[danielcdh]

lib/Transforms/IPO/SampleProfile.cpp

1439

Updated the comment to explain why we remove the existing entry.

[davidxl]

lib/Transforms/IPO/SampleProfile.cpp

1439

but the new entry is not inserted either here. Should it just reset r.second = F? Also In presence of conflict, how do we know which one to pick?

[danielcdh]

update

lib/Transforms/IPO/SampleProfile.cpp

1439

When there is conflict, we shall set the corresponding symbolmap entry to nullptr to avoid promoting random target.
Updated the code and test.

[davidxl]

ok. The original erase is fine (given the intention is to discard cases with conflicts).

lgtm

lib/Transforms/IPO/SampleProfile.cpp

 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Module.h"
+#include "llvm/IR/ValueSymbolTable.h"
 #include "llvm/Pass.h"
 #include "llvm/ProfileData/InstrProf.h"
 #include "llvm/ProfileData/SampleProfReader.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/raw_ostream.h"
protected:
   /// \brief Equivalence classes for block weights.
   ///
   /// Two blocks BB1 and BB2 are in the same equivalence class if they
   /// dominate and post-dominate each other, and they are in the same loop
   /// nest. When this happens, the two blocks are guaranteed to execute
   /// the same number of times.
   EquivalenceClassMap EquivalenceClass;
 
+  /// Map from function name to Function *. Used to find the function from
+  /// the function name. If the function name contains suffix, additional
+  /// entry is added to map from the stripped name to the function if there
+  /// is one-to-one mapping.
+  StringMap<Function *> SymbolMap;
+
   /// \brief Dominance, post-dominance and loop information.
   std::unique_ptr<DominatorTree> DT;
   std::unique_ptr<DominatorTreeBase<BasicBlock>> PDT;
   std::unique_ptr<LoopInfo> LI;
 
   AssumptionCacheTracker *ACT;
 
   /// \brief Predecessors for each basic block in the CFG.
for (auto I : CIS) {
       InlineFunctionInfo IFI(nullptr, ACT ? &GetAssumptionCache : nullptr);
       Function *CalledFunction = CallSite(I).getCalledFunction();
       Instruction *DI = I;
       if (!CalledFunction && !PromotedInsns.count(I) &&
           CallSite(I).isIndirectCall())
         for (const auto *FS : findIndirectCallFunctionSamples(*I)) {
           auto CalleeFunctionName = FS->getName();
           const char *Reason = "Callee function not available";
-          CalledFunction = F.getParent()->getFunction(CalleeFunctionName);
+          auto R = SymbolMap.find(CalleeFunctionName);
+          if (R == SymbolMap.end())
+            continue;
+          CalledFunction = R->getValue();
           if (CalledFunction && isLegalToPromote(I, CalledFunction, &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.
             DI = dyn_cast<Instruction>(
                 promoteIndirectCall(I, CalledFunction, 80, 100, false)
                     ->stripPointerCasts());

 bool SampleProfileLoader::runOnModule(Module &M) {
   if (!ProfileIsValid)
     return false;
 
   // Compute the total number of samples collected in this profile.
   for (const auto &I : Reader->getProfiles())
     TotalCollectedSamples += I.second.getTotalSamples();
 
+  // Populate the symbol map.
+  for (const auto &N_F : M.getValueSymbolTable()) {
+    std::string OrigName = N_F.getKey();
+    Function *F = dyn_cast<Function>(N_F.getValue());
+    if (F == nullptr)
+      continue;
+    SymbolMap[OrigName] = F;
+    auto pos = OrigName.find('.');
+    if (pos != std::string::npos) {
+      std::string NewName = OrigName.substr(0, pos);
+      auto r = SymbolMap.insert(std::make_pair(NewName, F));
+      // Failiing to insert means there is already an entry in SymbolMap,
+      // thus there are multiple functions that are mapped to the same
+      // stripped name. In this case of name conflicting, set the value
+      // to nullptr to avoid confusion.
+      if (!r.second)
+        r.first->second = nullptr;
+    }
+  }
+
   bool retval = false;
   for (auto &F : M)
     if (!F.isDeclaration()) {
       clearFunctionData();
       retval |= runOnFunction(F);
     }
   if (M.getProfileSummary() == nullptr)
     M.setProfileSummary(Reader->getSummary().getMD(M.getContext()));

test/Transforms/SampleProfile/Inputs/indirect-call.prof

 test:63067:0
  4: 3345 _Z3barv:1398 _Z3foov:2059
 test_inline:3000:0
  5: foo_inline1:3000
   1: 3000
  5: foo_inline2:4000
   1: 4000
 test_noinline:3000:0
  5: foo_noinline:3000
   1: 3000
 test_direct:3000:0
  5: foo_direct:3000
   1: 3000
+test_inline_strip:3000:0
+ 5: foo_inline_strip:3000
+  1: 3000
+test_inline_strip_confilict:3000:0
+ 5: foo_inline_strip_conflict:3000
+  1: 3000

test/Transforms/SampleProfile/indirect-call.ll

 ; CHECK: if.true.direct_targ1:
 ; CHECK-NOT: call
 ; CHECK: if.false.orig_indirect2:
 ; CHECK: call
   call i64* %3(i32* %x), !dbg !5
   ret void
 }
 
+; CHECK-LABEL: @test_inline_strip
+; If the indirect call is promoted and inlined in profile, and the callee name
+; is stripped we should promote and inline it.
+define void @test_inline_strip(i64* (i32*)*, i32* %x) !dbg !3 {
+  %2 = alloca i64* (i32*)*
+  store i64* (i32*)* %0, i64* (i32*)** %2
+  %3 = load i64* (i32*)*, i64* (i32*)** %2
+; CHECK: icmp {{.*}} @foo_inline_strip.suffix
+; CHECK: if.true.direct_targ:
+; CHECK-NOT: call
+; CHECK: if.false.orig_indirect:
+; CHECK: call
+  call i64* %3(i32* %x), !dbg !5
+  ret void
+}
+
+; CHECK-LABEL: @test_inline_strip_conflict
+; If the indirect call is promoted and inlined in profile, and the callee name
+; is stripped, but have more than 1 potential match, we should not promote.
+define void @test_inline_strip_conflict(i64* (i32*)*, i32* %x) !dbg !3 {
+  %2 = alloca i64* (i32*)*
+  store i64* (i32*)* %0, i64* (i32*)** %2
+  %3 = load i64* (i32*)*, i64* (i32*)** %2
+; CHECK-NOT: if.true.direct_targ:
+  call i64* %3(i32* %x), !dbg !5
+  ret void
+}
+
 ; CHECK-LABEL: @test_noinline
 ; If the indirect call target is not available, we should not promote it.
 define void @test_noinline(void ()*) !dbg !3 {
   %2 = alloca void ()*
   store void ()* %0, void ()** %2
   %3 = load void ()*, void ()** %2
 ; CHECK-NOT: icmp
 ; CHECK: call
   call void %3(), !dbg !5
   ret void
 }
 
 @x = global i32 0, align 4
 
 define i32* @foo_inline1(i32* %x) !dbg !3 {
   ret i32* %x
 }
 
+define i32* @foo_inline_strip.suffix(i32* %x) !dbg !3 {
+  ret i32* %x
+}
+
+define i32* @foo_inline_strip_conflict.suffix1(i32* %x) !dbg !3 {
+  ret i32* %x
+}
+
+define i32* @foo_inline_strip_conflict.suffix2(i32* %x) !dbg !3 {
+  ret i32* %x
+}
+
+define i32* @foo_inline_strip_conflict.suffix3(i32* %x) !dbg !3 {
+  ret i32* %x
+}
+
 define i32* @foo_inline2(i32* %x) !dbg !3 {
   ret i32* %x
 }
 
 define i32 @foo_noinline(i32 %x) !dbg !3 {
   ret i32 %x
 }