Revert "[PGO] Turn off comdat renaming in IR PGO by default"

This patch reverts r291588: [PGO] Turn off comdat renaming in IR PGO by default,
as we are seeing some hash mismatches in our internal tests.

llvm-svn: 291621

3 files changed, 15 insertions, 60 deletions

diff --git a/llvm/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp
index 74acd9e5e20..77c6ffc9c25 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -811,47 +811,4 @@ bool needsComdatForCounter(const Function &F, const Module &M) {
 
   return true;
 }
-
-// Check if INSTR_PROF_RAW_VERSION_VAR is defined.
-bool isIRPGOFlagSet(const Module *M) {
-  auto IRInstrVar =
-      M->getNamedGlobal(INSTR_PROF_QUOTE(INSTR_PROF_RAW_VERSION_VAR));
-  if (!IRInstrVar || IRInstrVar->isDeclaration() ||
-      IRInstrVar->hasLocalLinkage())
-    return false;
-
-  // Check if the flag is set.
-  if (!IRInstrVar->hasInitializer())
-    return false;
-
-  const Constant *InitVal = IRInstrVar->getInitializer();
-  if (!InitVal)
-    return false;
-
-  return (dyn_cast<ConstantInt>(InitVal)->getZExtValue() &
-          VARIANT_MASK_IR_PROF) != 0;
-}
-
-// Check if we can safely rename this Comdat function.
-bool canRenameComdatFunc(const Function &F, bool CheckAddressTaken) {
-  if (F.getName().empty())
-    return false;
-  if (!needsComdatForCounter(F, *(F.getParent())))
-    return false;
-  // Unsafe to rename the address-taken function (which can be used in
-  // function comparison).
-  if (CheckAddressTaken && F.hasAddressTaken())
-    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;
-  }
-  return true;
-}
 } // end namespace llvm
diff --git a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
index adea7e77244..8da3e31200f 100644
--- a/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
+++ b/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp
@@ -32,11 +32,6 @@ cl::opt<bool> DoNameCompression("enable-name-compression",
                                 cl::desc("Enable name string compression"),
                                 cl::init(true));
 
-cl::opt<bool> DoHashBasedCounterSplit(
-    "hash-based-counter-split",
-    cl::desc("Rename counter variable of a comdat function based on cfg hash"),
-    cl::init(true));
-
 cl::opt<bool> ValueProfileStaticAlloc(
     "vp-static-alloc",
     cl::desc("Do static counter allocation for value profiler"),
@@ -277,16 +272,7 @@ void InstrProfiling::lowerCoverageData(GlobalVariable *CoverageNamesVar) {
 static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix) {
   StringRef NamePrefix = getInstrProfNameVarPrefix();
   StringRef Name = Inc->getName()->getName().substr(NamePrefix.size());
-  Function *F = Inc->getParent()->getParent();
-  Module *M = F->getParent();
-  if (!DoHashBasedCounterSplit || !isIRPGOFlagSet(M) ||
-      !canRenameComdatFunc(*F))
-    return (Prefix + Name).str();
-  uint64_t FuncHash = Inc->getHash()->getZExtValue();
-  SmallVector<char, 24> HashPostfix;
-  if (Name.endswith((Twine(".") + Twine(FuncHash)).toStringRef(HashPostfix)))
-    return (Prefix + Name).str();
-  return (Prefix + Name + "." + Twine(FuncHash)).str();
+  return (Prefix + Name).str();
 }
 
 static inline bool shouldRecordFunctionAddr(Function *F) {
diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
index ffebd42795d..28f4f7ea145 100644
--- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
+++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
@@ -119,7 +119,7 @@ static cl::opt<unsigned> MaxNumAnnotations(
 // 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(false), cl::Hidden,
+    "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"));
 
@@ -407,8 +407,20 @@ void FuncPGOInstrumentation<Edge, BBInfo>::computeCFGHash() {
 static bool canRenameComdat(
     Function &F,
     std::unordered_multimap<Comdat *, GlobalValue *> &ComdatMembers) {
-  if (!DoComdatRenaming || !canRenameComdatFunc(F, true))
+  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.