[PGO] Fix profile mismatch in Comdat function with pre-inliner

[xur]

Summary

Pre-instrumentation inline (pre-inliner) greatly improves the IR instrumentation code performance, among other benefits. One issue of the pre-inliner is it can introduce CFG-mismatch for COMDAT functions. This is due to the fact that the same COMDAT function may have different early inline decisions across different modules -- that means different copies of COMDAT functions will have different CFG checksum.

A simple example:

COMDAT function foo() --> bar()

foo() is defined in f1.cc and f2.cc. If bar() is available in f1.cc and is small enough to inline in pre-inliner, and it's not available in f2.cc. After pre-inline pass, f1.cc and f2.cc each has its own foo() with different IR (different CFG). If the version in f1.cc is chosen, we only have this version of counters in the profile (as we merge the COMDAT profile variables). We will have checksum mismatch when do a profile-use compilation in f2.cc.

In this patch, we propose a partially renaming the COMDAT group and its member function/variable so we have different profile counter for each version. We will post-fix the COMDAT function and the group name with its FunctionHash.

For the above case, if the foo() in f1.cc has a hash of 123456 and the foo() in f2.cc has a hash of 234567. foo() in f1.cc will be rename to foo.123456() (as well as the COMDAT name, and the profile variables in that function). foo() in f2.cc will be rename to foo.234567() (as well as the COMDAT name, and the profile variables in that function).

There are cases where two functions with the same FunctionHash might not have the same IR. For example,
foo() { bar(); goo(); }
bar() { a++; }
goo() { b++; }
in f1.cc bar() is inlined while in f2.cc goo() is inlined. From edge profile point of view, nothing will go wrong as both version has the same profile variables and all the counter updates will be captured.

The only potential problem is the indirect-call profiling. For example, if bar() contains an indirect-call. The version of foo() in f1.cc will also contain an indirect-call counter, while the version in f2.cc does not have. This will create a mismatch when reading value profiles. To address this, we add the number of indirect-calls to the function hash.

This is not bullet-proof solution. As same number of indirect-calls does not guarantee the indirect-calls are the same. For example, if goo() contains a different indirect-call. Both version in f1.cc and f2.cc has one indirect call, even though they have different call-sites. Our proposed method will treat the two versions of foo() as identical as they have the same function hash. But we believe this happens rarely. Also note that the worse case here is that we will promote a wrong indirect-target. There are not correctness issues.

Some implementation details:
(1) The mismatch also applies to AvailableExternallyLinkage functions.
We will convert AvailableExternallyLinkage functions to COMDAT functions, but not ExternalWeakLinkage functions.

(2) COMDAT group with multiple functions.
We only handle COMDAT group that having one COMDAT function to reduce the complexity. If a COMDAT group has multiple functions, we need to have a unique post-fix for all the functions. To do this, it requires to collect all the member functions and their hash, which is costly.

An alternate way is to do a post instrumentation fix-up on the instrumentation intrinsics. This is costly too.

We find multiple functions COMDAT groups are relatively rare (mostly in global static initializer). So we decided to only handle single member COMDAT groups.

Future work:
(1) Reduce the number of renamings
One optimization that reduces the number of renamings is to only apply the renaming to the COMDAT functions that preinline occurs. Unfortunately, currently we does not have an attribute for this.

(2) Darwin
Darwin does not use COMDAT, instead, it uses LinkOnce linkage. We will have a separated patch to deal the mismatch introduced by pre-inliner.

[silvas]

adding Jake

[jakev]

Just a couple nits from me.

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

121 ↗

(On Diff #64772)

Does this need ZeroOrMore?

122 ↗

(On Diff #64772)

functin -> function

286 ↗

(On Diff #64772)

Can this just be ComdatMembers.size()?

325 ↗

(On Diff #64772)

Can this be called 'canRenameComdat'?

888 ↗

(On Diff #64772)

comdat.in -> comdat in

[silvas]

I think this is a good solution. LGTM although I'd like to wait for other reviewers to chime in.

FWIW, one alternative solution to this issue (and the static var name issue that Jake fixed recently) is to make it so that multiple functions with same name and different hash can coexist naturally. I haven't looked closely, but this would require changing quite a bit (e.g. indexed format, various API's, etc.). So at the moment I don't think it is worth it since this patch + Jake's patch are relatively small.

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

988 ↗

(On Diff #64772)

Is this variable Builder needed?

[xur]

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

121 ↗

(On Diff #64772)

Should be Optional. Removed ZeroOrMore.

286 ↗

(On Diff #64772)

good suggestion. changed.

325 ↗

(On Diff #64772)

changed.

988 ↗

(On Diff #64772)

This is from a leftover of earlier merge. It should not be here. Thanks for catching this.

[xur]

Integrated the review suggestions from Jake and Sean.

Thanks,

-Rong

[davidxl]

lib/ProfileData/InstrProf.cpp

784 ↗

(On Diff #64931)

Please separate the restructuring into a NFC patch

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

120 ↗

(On Diff #64931)

Change the name to "DoComdatRenaming"

[xur]

Address David's review comments:
(1) change option name.
(2) split the refactoring work into NFC patch:
https://reviews.llvm.org/D22643
D22643 [PGO] Make needsComdatForCounter() available (NFC)

[davidxl]

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

338 ↗

(On Diff #64946)

Perhaps add an assert about the linkage?

341 ↗

(On Diff #64946)

Add // FIXME and some explanation of the limitation.

342 ↗

(On Diff #64946)

have have --> have

344 ↗

(On Diff #64946)

Also explain why having other variables are ok?

365 ↗

(On Diff #64946)

--> change the linkage to LinkOnceODR and put them into comdat. This is because after renaming, there is no backup external copy available for the function.

367 ↗

(On Diff #64946)

Add assertion about availableExternally linkage.

test/Transforms/PGOProfile/comdat_internal.ll

7 ↗

(On Diff #64946)

Can you use wild card and defined a variable for the hash val? There is no need to test the actual alue.

test/Transforms/PGOProfile/indirect_call_profile.ll

16 ↗

(On Diff #64946)

Same here

[xur]

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

344 ↗

(On Diff #64946)

It turns out having variable in the comdat group is not safe to rename: if we rename the variable, we will create different copy of variable which is wrong. if we don't rename, we might run into duplicated symbol in linking.
So in the new patch, I disable the rename of the comdat groups with variables.

It's ok to have aliases though.

[xur]

This new patch addressed David's comments:
It refines the handling of comdat group with variables and aliases, and adds a test case to check handled and unhandled cases.

[davidxl]

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

348 ↗

(On Diff #64997)

(2) variables can not be renamed, so we can not rename comdat function in a group including global vars.

393 ↗

(On Diff #64997)

Add assert that the alias target is still F

test/Transforms/PGOProfile/comdat_rename.ll

2 ↗

(On Diff #64997)

you may want to try this test case on other platform such as coff to make sure it works:

-mtriple=x86_64-pc-win32-coff

[xur]

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

393 ↗

(On Diff #64997)

I'll assert GA's comdat is the same as OrigComdat.

test/Transforms/PGOProfile/comdat_rename.ll

2 ↗

(On Diff #64997)

The test failed for the AvailableExternallyLinkage case. The reason is in needsComdatForCounter().

For AvailableExternallyLinkage, since this is not ELF format target, we return false.

I'm wondering if we should move the checking for TargetTriple() after the linkage check. The target can have Comdat support for nonELF targets after all.

[davidxl]

test/Transforms/PGOProfile/comdat_rename.ll

2 ↗

(On Diff #64997)

you should explicitly add -mtripple (covering elf and coff). For coff case, do not check availableExternally (which is not supported yet with this change).

[xur]

Integrated David's comments.

Thanks,

-Rong

[davidxl]

As a follow up, perfhaps a new internal option can be introduced to force 'privatize a comdat function': when a comdat function has module context specific profile, it is can be useful to let it have its own copy of profile counters.

lgtm

[silvas]

Thanks David for the close review! (and of course thanks to Rong for the patch!)

llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp

 // used in populating profile counters.
 // The MST implementation is in Class CFGMST (CFGMST.h).
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Transforms/PGOInstrumentation.h"
 #include "CFGMST.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Analysis/BlockFrequencyInfo.h"
 #include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/Analysis/CFG.h"
 #include "llvm/Analysis/IndirectCallSiteVisitor.h"
 #include "llvm/IR/CallSite.h"
 #include "llvm/IR/DiagnosticInfo.h"
+#include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/IR/InstIterator.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Pass.h"
 #include "llvm/ProfileData/InstrProfReader.h"
 #include "llvm/ProfileData/ProfileCommon.h"
 #include "llvm/Support/BranchProbability.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/JamCRC.h"
 #include "llvm/Transforms/Instrumentation.h"
 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include <algorithm>
 #include <string>
+#include <unordered_map>
 #include <utility>
 #include <vector>
 
 using namespace llvm;
 
 #define DEBUG_TYPE "pgo-instrumentation"
 
 STATISTIC(NumOfPGOInstrument, "Number of edges instrumented.");

 
 // Command line option to set the maximum number of VP annotations to write to
 // the metadata for a single indirect call callsite.
 static cl::opt<unsigned> MaxNumAnnotations(
     "icp-max-annotations", cl::init(3), cl::Hidden, cl::ZeroOrMore,
     cl::desc("Max number of annotations for a single indirect "
              "call callsite"));
 
+// Command line option to control appending FunctionHash to the name of a COMDAT
+// function. This is to avoid the hash mismatch caused by the preinliner.
+static cl::opt<bool> DoComdatRenaming(
+    "do-comdat-renaming", cl::init(true), cl::Hidden,
+    cl::desc("Append function hash to the name of COMDAT function to avoid "
+             "function hash mismatch due to the preinliner"));
+
 // Command line option to enable/disable the warning about missing profile
 // information.
 static cl::opt<bool> NoPGOWarnMissing("no-pgo-warn-missing", cl::init(false),
                                       cl::Hidden);
 
 // Command line option to enable/disable the warning about a hash mismatch in
 // the profile data.
 static cl::opt<bool> NoPGOWarnMismatch("no-pgo-warn-mismatch", cl::init(false),
struct BBInfo {
   }
 };
 
 // This class implements the CFG edges. Note the CFG can be a multi-graph.
 template <class Edge, class BBInfo> class FuncPGOInstrumentation {
 private:
   Function &F;
   void computeCFGHash();
+  void renameComdatFunction();
+  // A map that stores the Comdat group in function F.
+  std::unordered_multimap<Comdat *, GlobalValue *> &ComdatMembers;
 
 public:
   std::string FuncName;
   GlobalVariable *FuncNameVar;
   // CFG hash value for this function.
   uint64_t FunctionHash;
 
   // The Minimum Spanning Tree of function CFG.
   CFGMST<Edge, BBInfo> MST;
 
   // Give an edge, find the BB that will be instrumented.
   // Return nullptr if there is no BB to be instrumented.
   BasicBlock *getInstrBB(Edge *E);
 
   // Return the auxiliary BB information.
   BBInfo &getBBInfo(const BasicBlock *BB) const { return MST.getBBInfo(BB); }
 
   // Dump edges and BB information.
   void dumpInfo(std::string Str = "") const {
     MST.dumpEdges(dbgs(), Twine("Dump Function ") + FuncName + " Hash: " +
                               Twine(FunctionHash) + "\t" + Str);
   }
 
-  FuncPGOInstrumentation(Function &Func, bool CreateGlobalVar = false,
-                         BranchProbabilityInfo *BPI = nullptr,
-                         BlockFrequencyInfo *BFI = nullptr)
-      : F(Func), FunctionHash(0), MST(F, BPI, BFI) {
+  FuncPGOInstrumentation(
+      Function &Func,
+      std::unordered_multimap<Comdat *, GlobalValue *> &ComdatMembers,
+      bool CreateGlobalVar = false, BranchProbabilityInfo *BPI = nullptr,
+      : F(Func), ComdatMembers(ComdatMembers), FunctionHash(0),
+        MST(F, BPI, BFI) {
     FuncName = getPGOFuncName(F);
     computeCFGHash();
+    if (ComdatMembers.size())
+      renameComdatFunction();
     DEBUG(dumpInfo("after CFGMST"));
 
     NumOfPGOBB += MST.BBInfos.size();
     for (auto &E : MST.AllEdges) {
       if (E->Removed)
         continue;
       NumOfPGOEdge++;
       if (!E->InMST)
for (auto &BB : F) {
     for (unsigned I = 0, E = TI->getNumSuccessors(); I != E; ++I) {
       BasicBlock *Succ = TI->getSuccessor(I);
       uint32_t Index = getBBInfo(Succ).Index;
       for (int J = 0; J < 4; J++)
         Indexes.push_back((char)(Index >> (J * 8)));
     }
   }
   JC.update(Indexes);
-  FunctionHash = (uint64_t)MST.AllEdges.size() << 32 | JC.getCRC();
+  FunctionHash = (uint64_t)findIndirectCallSites(F).size() << 48 |
+                 (uint64_t)MST.AllEdges.size() << 32 | JC.getCRC();
+}
+
+// Check if we can safely rename this Comdat function.
+static bool canRenameComdat(
+    Function &F,
+    std::unordered_multimap<Comdat *, GlobalValue *> &ComdatMembers) {
+  if (F.getName().empty())
+    return false;
+  if (!needsComdatForCounter(F, *(F.getParent())))
+    return false;
+  // Only safe to do if this function may be discarded if it is not used
+  // in the compilation unit.
+  if (!GlobalValue::isDiscardableIfUnused(F.getLinkage()))
+    return false;
+
+  // For AvailableExternallyLinkage functions.
+  if (!F.hasComdat()) {
+    assert(F.getLinkage() == GlobalValue::AvailableExternallyLinkage);
+    return true;
+  }
+
+  // FIXME: Current only handle those Comdat groups that only containing one
+  // function and function aliases.
+  // (1) For a Comdat group containing multiple functions, we need to have a
+  // unique postfix based on the hashes for each function. There is a
+  // non-trivial code refactoring to do this efficiently.
+  // (2) Variables can not be renamed, so we can not rename Comdat function in a
+  // group including global vars.
+  Comdat *C = F.getComdat();
+  for (auto &&CM : make_range(ComdatMembers.equal_range(C))) {
+    if (dyn_cast<GlobalAlias>(CM.second))
+      continue;
+    Function *FM = dyn_cast<Function>(CM.second);
+    if (FM != &F)
+      return false;
+  }
+  return true;
+}
+
+// Append the CFGHash to the Comdat function name.
+template <class Edge, class BBInfo>
+void FuncPGOInstrumentation<Edge, BBInfo>::renameComdatFunction() {
+  if (!canRenameComdat(F, ComdatMembers))
+    return;
+  std::string NewFuncName =
+      Twine(F.getName() + "." + Twine(FunctionHash)).str();
+  F.setName(Twine(NewFuncName));
+  FuncName = Twine(FuncName + "." + Twine(FunctionHash)).str();
+  Comdat *NewComdat;
+  Module *M = F.getParent();
+  // For AvailableExternallyLinkage functions, change the linkage to
+  // LinkOnceODR and put them into comdat. This is because after renaming, there
+  // is no backup external copy available for the function.
+  if (!F.hasComdat()) {
+    assert(F.getLinkage() == GlobalValue::AvailableExternallyLinkage);
+    NewComdat = M->getOrInsertComdat(StringRef(NewFuncName));
+    F.setLinkage(GlobalValue::LinkOnceODRLinkage);
+    F.setComdat(NewComdat);
+    return;
+  }
+
+  // This function belongs to a single function Comdat group.
+  Comdat *OrigComdat = F.getComdat();
+  std::string NewComdatName =
+      Twine(OrigComdat->getName() + "." + Twine(FunctionHash)).str();
+  NewComdat = M->getOrInsertComdat(StringRef(NewComdatName));
+  NewComdat->setSelectionKind(OrigComdat->getSelectionKind());
+
+  for (auto &&CM : make_range(ComdatMembers.equal_range(OrigComdat))) {
+    if (GlobalAlias *GA = dyn_cast<GlobalAlias>(CM.second)) {
+      // For aliases, change the name directly.
+      assert(dyn_cast<Function>(GA->getAliasee()->stripPointerCasts()) == &F);
+      GA->setName(Twine(GA->getName() + "." + Twine(FunctionHash)));
+      continue;
+    }
+    // Must be a function.
+    Function *CF = dyn_cast<Function>(CM.second);
+    assert(CF);
+    CF->setComdat(NewComdat);
+  }
 }
 
 // Given a CFG E to be instrumented, find which BB to place the instrumented
 // code. The function will split the critical edge if necessary.
 template <class Edge, class BBInfo>
 BasicBlock *FuncPGOInstrumentation<Edge, BBInfo>::getInstrBB(Edge *E) {
   if (E->InMST || E->Removed)
     return nullptr;
BasicBlock *FuncPGOInstrumentation<Edge, BBInfo>::getInstrBB(Edge *E) {
   assert(InstrBB && "Critical edge is not split");
 
   E->Removed = true;
   return InstrBB;
 }
 
 // Visit all edge and instrument the edges not in MST, and do value profiling.
 // Critical edges will be split.
-static void instrumentOneFunc(Function &F, Module *M,
-                              BranchProbabilityInfo *BPI,
-                              BlockFrequencyInfo *BFI) {
+static void instrumentOneFunc(
+    Function &F, Module *M, BranchProbabilityInfo *BPI, BlockFrequencyInfo *BFI,
+    std::unordered_multimap<Comdat *, GlobalValue *> &ComdatMembers) {
   unsigned NumCounters = 0;
-  FuncPGOInstrumentation<PGOEdge, BBInfo> FuncInfo(F, true, BPI, BFI);
+  FuncPGOInstrumentation<PGOEdge, BBInfo> FuncInfo(F, ComdatMembers, true, BPI,
+                                                   BFI);
   for (auto &E : FuncInfo.MST.AllEdges) {
     if (!E->InMST && !E->Removed)
       NumCounters++;
   }
-
   uint32_t I = 0;
   Type *I8PtrTy = Type::getInt8PtrTy(M->getContext());
   for (auto &E : FuncInfo.MST.AllEdges) {
     BasicBlock *InstrBB = FuncInfo.getInstrBB(E.get());
     if (!InstrBB)
       continue;
 
     IRBuilder<> Builder(InstrBB, InstrBB->getFirstInsertionPt());
if (E->Removed)
       continue;
     Total += E->CountValue;
   }
   return Total;
 }
 
 class PGOUseFunc {
 public:
-  PGOUseFunc(Function &Func, Module *Modu, BranchProbabilityInfo *BPI = nullptr,
+  PGOUseFunc(Function &Func, Module *Modu,
+             std::unordered_multimap<Comdat *, GlobalValue *> &ComdatMembers,
+             BranchProbabilityInfo *BPI = nullptr,
              BlockFrequencyInfo *BFI = nullptr)
-      : F(Func), M(Modu), FuncInfo(Func, false, BPI, BFI),
+      : F(Func), M(Modu), FuncInfo(Func, ComdatMembers, false, BPI, BFI),
         FreqAttr(FFA_Normal) {}
 
   // Read counts for the instrumented BB from profile.
   bool readCounters(IndexedInstrProfReader *PGOReader);
 
   // Populate the counts for all BBs.
   void populateCounters();
 
   // Set the branch weights based on the count values.
   void setBranchWeights();
 
   // Annotate the indirect call sites.
   void annotateIndirectCallSites();
 
   // The hotness of the function from the profile count.
   enum FuncFreqAttr { FFA_Normal, FFA_Cold, FFA_Hot };
 
   // Return the function hotness from the profile.
   FuncFreqAttr getFuncFreqAttr() const { return FreqAttr; }
 
+  // Return the function hash.
+  uint64_t getFuncHash() const { return FuncInfo.FunctionHash; }
   // Return the profile record for this function;
   InstrProfRecord &getProfileRecord() { return ProfileRecord; }
 
 private:
   Function &F;
   Module *M;
   // This member stores the shared information with class PGOGenFunc.
   FuncPGOInstrumentation<PGOUseEdge, UseBBInfo> FuncInfo;
static void createIRLevelProfileFlagVariable(Module &M) {
   Triple TT(M.getTargetTriple());
   if (!TT.supportsCOMDAT())
     IRLevelVersionVariable->setLinkage(GlobalValue::WeakAnyLinkage);
   else
     IRLevelVersionVariable->setComdat(M.getOrInsertComdat(
         StringRef(INSTR_PROF_QUOTE(INSTR_PROF_RAW_VERSION_VAR))));
 }
 
+// Collect the set of members for each Comdat in module M and store
+// in ComdatMembers.
+static void collectComdatMembers(
+    Module &M,
+    std::unordered_multimap<Comdat *, GlobalValue *> &ComdatMembers) {
+  if (!DoComdatRenaming)
+    return;
+  for (Function &F : M)
+    if (Comdat *C = F.getComdat())
+      ComdatMembers.insert(std::make_pair(C, &F));
+  for (GlobalVariable &GV : M.globals())
+    if (Comdat *C = GV.getComdat())
+      ComdatMembers.insert(std::make_pair(C, &GV));
+  for (GlobalAlias &GA : M.aliases())
+    if (Comdat *C = GA.getComdat())
+      ComdatMembers.insert(std::make_pair(C, &GA));
+}
+
 static bool InstrumentAllFunctions(
     Module &M, function_ref<BranchProbabilityInfo *(Function &)> LookupBPI,
     function_ref<BlockFrequencyInfo *(Function &)> LookupBFI) {
   createIRLevelProfileFlagVariable(M);
+  std::unordered_multimap<Comdat *, GlobalValue *> ComdatMembers;
+  collectComdatMembers(M, ComdatMembers);
+
   for (auto &F : M) {
     if (F.isDeclaration())
       continue;
     auto *BPI = LookupBPI(F);
     auto *BFI = LookupBFI(F);
-    instrumentOneFunc(F, &M, BPI, BFI);
+    instrumentOneFunc(F, &M, BPI, BFI, ComdatMembers);
   }
   return true;
 }
 
 bool PGOInstrumentationGenLegacyPass::runOnModule(Module &M) {
   if (skipModule(M))
     return false;
 
static bool annotateAllFunctions(
   }
   // TODO: might need to change the warning once the clang option is finalized.
   if (!PGOReader->isIRLevelProfile()) {
     Ctx.diagnose(DiagnosticInfoPGOProfile(
         ProfileFileName.data(), "Not an IR level instrumentation profile"));
     return false;
   }
 
+  std::unordered_multimap<Comdat *, GlobalValue *> ComdatMembers;
+  collectComdatMembers(M, ComdatMembers);
   std::vector<Function *> HotFunctions;
   std::vector<Function *> ColdFunctions;
   for (auto &F : M) {
     if (F.isDeclaration())
       continue;
     auto *BPI = LookupBPI(F);
     auto *BFI = LookupBFI(F);
-    PGOUseFunc Func(F, &M, BPI, BFI);
+    PGOUseFunc Func(F, &M, ComdatMembers, BPI, BFI);
     if (!Func.readCounters(PGOReader.get()))
       continue;
     Func.populateCounters();
     Func.setBranchWeights();
     Func.annotateIndirectCallSites();
     PGOUseFunc::FuncFreqAttr FreqAttr = Func.getFuncFreqAttr();
     if (FreqAttr == PGOUseFunc::FFA_Cold)
       ColdFunctions.push_back(&F);
for (auto &F : HotFunctions) {
     F->addFnAttr(llvm::Attribute::InlineHint);
     DEBUG(dbgs() << "Set inline attribute to function: " << F->getName()
                  << "\n");
   }
   for (auto &F : ColdFunctions) {
     F->addFnAttr(llvm::Attribute::Cold);
     DEBUG(dbgs() << "Set cold attribute to function: " << F->getName() << "\n");
   }
-
   return true;
 }
 
 PGOInstrumentationUse::PGOInstrumentationUse(std::string Filename)
     : ProfileFileName(std::move(Filename)) {
   if (!PGOTestProfileFile.empty())
     ProfileFileName = PGOTestProfileFile;
 }

llvm/trunk/test/Transforms/PGOProfile/Inputs/indirect_call.proftext

 :ir
 bar
 # Func Hash:
-12884901887
[phone]
 # Num Counters:
 1
 # Counter Values:
 140
 # Num Value Kinds:
 1
 # ValueKind = IPVK_IndirectCallTarget:
 0

llvm/trunk/test/Transforms/PGOProfile/comdat_internal.ll

 ; RUN: opt < %s -pgo-instr-gen -instrprof -S | FileCheck %s
 ; RUN: opt < %s -passes=pgo-instr-gen,instrprof -S | FileCheck %s
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 $foo = comdat any
+; CHECK: $foo.[[FOO_HASH:[0-9]+]] = comdat any
 
 ; CHECK: $__llvm_profile_raw_version = comdat any
-; CHECK: $__profv__stdin__foo = comdat any
+; CHECK: $__profv__stdin__foo.[[FOO_HASH]] = comdat any
 
 @bar = global i32 ()* @foo, align 8
 
 ; CHECK: @__llvm_profile_raw_version = constant i64 {{[0-9]+}}, comdat
-; CHECK: @__profn__stdin__foo = private constant [11 x i8] c"<stdin>:foo"
-; CHECK: @__profc__stdin__foo = private global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat($__profv__stdin__foo), align 8
-; CHECK: @__profd__stdin__foo = private global { i64, i64, i64*, i8*, i8*, i32, [1 x i16] } { i64 -5640069336071256030, i64 12884901887, i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__stdin__foo, i32 0, i32 0), i8*
+; CHECK: @__profn__stdin__foo.[[FOO_HASH]] = private constant [23 x i8] c"<stdin>:foo.[[FOO_HASH]]"
+; CHECK: @__profc__stdin__foo.[[FOO_HASH]] = private global [1 x i64] zeroinitializer, section "__llvm_prf_cnts", comdat($__profv__stdin__foo.[[FOO_HASH]]), align 8
+; CHECK: @__profd__stdin__foo.[[FOO_HASH]] = private global { i64, i64, i64*, i8*, i8*, i32, [1 x i16] } { i64 6965568665848889497, i64 [[FOO_HASH]], i64* getelementptr inbounds ([1 x i64], [1 x i64]* @__profc__stdin__foo.[[FOO_HASH]], i32 0, i32 0), i8* null
 ; CHECK-NOT: bitcast (i32 ()* @foo to i8*)
-; CHECK-SAME: null
-; CHECK-SAME: , i8* null, i32 1, [1 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profv__stdin__foo), align 8
+; CHECK-SAME: , i8* null, i32 1, [1 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profv__stdin__foo.[[FOO_HASH]]), align 8
 ; CHECK: @__llvm_prf_nm
 ; CHECK: @llvm.used
 
 define internal i32 @foo() comdat {
 entry:
   ret i32 1
 }

llvm/trunk/test/Transforms/PGOProfile/comdat_rename.ll

+; RUN: opt < %s -mtriple=x86_64-unknown-linux -pgo-instr-gen -S | FileCheck --check-prefixes COMMON,ELFONLY %s
+; RUN: opt < %s -mtriple=x86_64-unknown-linux -passes=pgo-instr-gen -S | FileCheck --check-prefixes COMMON,ELFONLY %s
+; RUN: opt < %s -mtriple=x86_64-pc-win32-coff -pgo-instr-gen -S | FileCheck --check-prefixes COMMON,COFFONLY %s
+; RUN: opt < %s -mtriple=x86_64-pc-win32-coff -passes=pgo-instr-gen -S | FileCheck --check-prefixes COMMON,COFFONLY %s
+
+; Rename Comdat group and its function.
+$f = comdat any
+; COMMON: $f.[[SINGLEBB_HASH:[0-9]+]] = comdat any
+define linkonce_odr void @f() comdat($f) {
+  ret void
+}
+
+; Not rename Comdat with right linkage.
+$nf = comdat any
+; COMMON: $nf = comdat any
+define void @nf() comdat($nf) {
+  ret void
+}
+
+; Not rename Comdat with variable members.
+$f_with_var = comdat any
+; COMMON: $f_with_var = comdat any
+@var = global i32 0, comdat($f_with_var)
+define linkonce_odr void @f_with_var() comdat($f_with_var) {
+  %tmp = load i32, i32* @var, align 4
+  %inc = add nsw i32 %tmp, 1
+  store i32 %inc, i32* @var, align 4
+  ret void
+}
+
+; Not rename Comdat with multiple functions.
+$tf = comdat any
+; COMMON: $tf = comdat any
+define linkonce void @tf() comdat($tf) {
+  ret void
+}
+define linkonce void @tf2() comdat($tf) {
+  ret void
+}
+
+; Renaming Comdat with aliases.
+$f_with_alias = comdat any
+; COMMON: $f_with_alias.[[SINGLEBB_HASH]] = comdat any
+@af = alias void (...), bitcast (void ()* @f_with_alias to void (...)*)
+; COFFONLY: @af.[[SINGLEBB_HASH]] = alias void (...), bitcast (void ()* @f_with_alias.[[SINGLEBB_HASH]] to
+; ELFONLY-DAG: @af.[[SINGLEBB_HASH]] = alias void (...), bitcast (void ()* @f_with_alias.[[SINGLEBB_HASH]] to
+define linkonce_odr void @f_with_alias() comdat($f_with_alias) {
+  ret void
+}
+
+; Rename AvailableExternallyLinkage functions
+; ELFONLY-DAG: $aef.[[SINGLEBB_HASH]] = comdat any
+define available_externally void @aef() {
+; ELFONLY: define linkonce_odr void @aef.[[SINGLEBB_HASH]]() comdat {
+; COFFONLY: define available_externally void @aef() {
+  ret void
+}
+
+

llvm/trunk/test/Transforms/PGOProfile/indirect_call_profile.ll

 ; RUN: opt < %s -pgo-instr-gen -S | FileCheck %s --check-prefix=GEN
 ; RUN: opt < %s -passes=pgo-instr-gen -S | FileCheck %s --check-prefix=GEN
 ; RUN: opt < %s -passes=pgo-instr-gen,instrprof -S | FileCheck %s --check-prefix=LOWER
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 $foo3 = comdat any
 
 @bar = external global void ()*, align 8
 ; GEN: @__profn_foo = private constant [3 x i8] c"foo"
 
 define void @foo() {
 entry:
 ; GEN: entry:
-; GEN-NEXT: call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 12884901887, i32 1, i32 0)
+; GEN-NEXT: call void @llvm.instrprof.increment(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 [[FOO_HASH:[0-9]+]], i32 1, i32 0)
   %tmp = load void ()*, void ()** @bar, align 8
 ; GEN: [[ICALL_TARGET:%[0-9]+]] = ptrtoint void ()* %tmp to i64
-; GEN-NEXT: call void @llvm.instrprof.value.profile(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 12884901887, i64 [[ICALL_TARGET]], i32 0, i32 0)
+; GEN-NEXT: call void @llvm.instrprof.value.profile(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @__profn_foo, i32 0, i32 0), i64 [[FOO_HASH]], i64 [[ICALL_TARGET]], i32 0, i32 0)
   call void %tmp()
   ret void
 }
 
 @bar2 = global void ()* null, align 8
 @_ZTIi = external constant i8*
 
 define i32 @foo2(i32 %arg, i8** nocapture readnone %arg1) personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 bb:
   %tmp2 = load void ()*, void ()** @bar2, align 8
   invoke void %tmp2()
           to label %bb10 unwind label %bb2
 ; GEN: [[ICALL_TARGET2:%[0-9]+]] = ptrtoint void ()* %tmp2 to i64
-; GEN-NEXT: call void @llvm.instrprof.value.profile(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @__profn_foo2, i32 0, i32 0), i64 38432627612, i64 [[ICALL_TARGET2]], i32 0, i32 0)
+; GEN-NEXT: call void @llvm.instrprof.value.profile(i8* getelementptr inbounds ([4 x i8], [4 x i8]* @__profn_foo2, i32 0, i32 0), i64 [[FOO2_HASH:[0-9]+]], i64 [[ICALL_TARGET2]], i32 0, i32 0)
 
 bb2:                                              ; preds = %bb
   %tmp3 = landingpad { i8*, i32 }
           catch i8* bitcast (i8** @_ZTIi to i8*)
   %tmp4 = extractvalue { i8*, i32 } %tmp3, 1
   %tmp5 = tail call i32 @llvm.eh.typeid.for(i8* bitcast (i8** @_ZTIi to i8*))
   %tmp6 = icmp eq i32 %tmp4, %tmp5
   br i1 %tmp6, label %bb7, label %bb11
 
 bb7:                                              ; preds = %bb2
   %tmp8 = extractvalue { i8*, i32 } %tmp3, 0
   %tmp9 = tail call i8* @__cxa_begin_catch(i8* %tmp8)
   tail call void @__cxa_end_catch()
   br label %bb10
 
 bb10:                                             ; preds = %bb7, %bb
   ret i32 0
 
 bb11:                                             ; preds = %bb2
   resume { i8*, i32 } %tmp3
 }
 
 ; Test that comdat function's address is recorded.
-; LOWER: @__profd_foo3 = linkonce_odr{{.*}}@foo3
+; LOWER: @__profd_foo3.[[FOO3_HASH:[0-9]+]] = linkonce_odr{{.*}}@foo3.[[FOO3_HASH]]
 ; Function Attrs: nounwind uwtable
 define linkonce_odr i32 @foo3()  comdat  {
   ret i32 1
 }
 
 declare i32 @__gxx_personality_v0(...)
 
 ; Function Attrs: nounwind readnone
 declare i32 @llvm.eh.typeid.for(i8*) #0
 
 declare i8* @__cxa_begin_catch(i8*)
 
 declare void @__cxa_end_catch()