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

[xur]

Summary

In IR PGO we append the function hash to comdat functions to avoid the
potential hash mismatch. This turns out not legal in some cases: if the comdat
function is address-taken and used in comparison. Renaming changes the semantic.

This patch turns off comdat renaming by default.

To alleviate the hash mismatch issue, we now rename the profile variable
for comdat functions. Profile allows co-existing multiple versions of profiles
with different hash value. The inlined copy will always has the correct profile
counter. The out-of-line copy might not have the correct count. But we will
not have the bogus mismatch warning.

[davidxl]

needs a profile use test case such that a comdat function has multiple copies of profile data and the profile annotator can find the one with the matching hash.

include/llvm/ProfileData/InstrProf.h

237

Add comments: instances of the same comdat function may have different control flows thus can not share the same counter variable.

238

canRenameComdatFunc

lib/Transforms/Instrumentation/InstrProfiling.cpp

35

-> DoHashBasedCounterSplit

37

"Rename counter variable of a comdat function based on cfg hash".

[xur]

D28416: [PGO] Turn off comdat renaming in IR PGO by default

Integrated David's review comments:
(1) added a multiple comdat copies test case.
(2) changed naming and comments.

[davidxl]

lib/ProfileData/InstrProf.cpp

839

Given the name of the function, it should probably check address taken bit as well.

lib/Transforms/Instrumentation/InstrProfiling.cpp

285

appent --> append

285

why there is on hash? or it is hash of a trivial function?

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

410

It is clearer to check DoComdatRenaming here as well.

[xur]

lib/ProfileData/InstrProf.cpp

839

We can do that, but it won't completely solve the issue as the address taken can happen in other TU.

lib/Transforms/Instrumentation/InstrProfiling.cpp

285

this can be called directly from intrinsic in a .ll file (happen some test cases), but since we now check the IRPGO bit, we can remove this.

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

410

I can add the check, but this essentially gonna be redundant: we only collect ComdatMembers under DoComdatRenaming. And we only call canRenameComdat when ComdatMembers is not empty.
So when DoComdatRenming is false, we will not call canRenameComdat.

[davidxl]

lib/ProfileData/InstrProf.cpp

839

Yes -- that is why it is better to add the check here and document 1) why it is not safe to be on by default 2) even when it is on, there is a risk as you mentioned.

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

410

Right -- it is clearer this way.

[xur]

updated patch
Integrated David's review comments.
Note I need to conditionally check addresstaken bit in
canRenameComdatFunc() as this function is also used in renaming profile
variable which does not care if address-taken or not.
Also checking this bit requries the update of one test case.

[davidxl]

lgtm

include/llvm/ProfileData/InstrProf.h

 Error collectPGOFuncNameStrings(const std::vector<GlobalVariable *> &NameVars,
                                 std::string &Result, bool doCompression = true);
 class InstrProfSymtab;
 /// \c NameStrings is a string composed of one of more sub-strings encoded in
 /// the format described above. The substrings are separated by 0 or more zero
 /// bytes. This method decodes the string and populates the \c Symtab.
 Error readPGOFuncNameStrings(StringRef NameStrings, InstrProfSymtab &Symtab);
 
+/// Check if INSTR_PROF_RAW_VERSION_VAR is defined. This global is only being
+/// set in IR PGO compilation.
+bool isIRPGOFlagSet(const Module *M);
+
+/// Check if we can safely rename this Comdat function.
+bool canRenameComdatInIRPGO(const Function &F);
+
 enum InstrProfValueKind : uint32_t {
 #define VALUE_PROF_KIND(Enumerator, Value) Enumerator = Value,
 #include "llvm/ProfileData/InstrProfData.inc"
 };
 
 struct InstrProfRecord;
 
 /// Get the value profile data for value site \p SiteIdx from \p InstrProfR

lib/ProfileData/InstrProf.cpp

bool needsComdatForCounter(const Function &F, const Module &M) {
   // will be accumulated by the profile merger.
   GlobalValue::LinkageTypes Linkage = F.getLinkage();
   if (Linkage != GlobalValue::ExternalWeakLinkage &&
       Linkage != GlobalValue::AvailableExternallyLinkage)
     return false;
 
   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 canRenameComdatInIRPGO(const Function &F) {
+  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;
+  }
+  return true;
+}
 } // end namespace llvm

lib/Transforms/Instrumentation/InstrProfiling.cpp

 #define DEBUG_TYPE "instrprof"
 
 namespace {
 
 cl::opt<bool> DoNameCompression("enable-name-compression",
                                 cl::desc("Enable name string compression"),
                                 cl::init(true));
 
+cl::opt<bool> DoInstrprofVarHashpostfix(
+    "instrprof-var-hashpostfix",
+    cl::desc("Append hash to the instrprof variables"), cl::init(true));
+
 cl::opt<bool> ValueProfileStaticAlloc(
     "vp-static-alloc",
     cl::desc("Do static counter allocation for value profiler"),
     cl::init(true));
 cl::opt<double> NumCountersPerValueSite(
     "vp-counters-per-site",
     cl::desc("The average number of profile counters allocated "
              "per value profiling site."),
for (unsigned I = 0, E = Names->getNumOperands(); I < E; ++I) {
     ReferencedNames.push_back(Name);
   }
 }
 
 /// Get the name of a profiling variable for a particular function.
 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 (!DoInstrprofVarHashpostfix || !isIRPGOFlagSet(M) ||
+      !canRenameComdatInIRPGO(*F))
+    return (Prefix + Name).str();
+  uint64_t FuncHash = Inc->getHash()->getZExtValue();
+  // No hash: don't appent it to the profile variable.
+  if (FuncHash == 0)
+    return (Prefix + Name).str();
+  SmallVector<char, 24> HashPostfix;
+  if (Name.endswith((Twine(".") + Twine(FuncHash)).toStringRef(HashPostfix)))
     return (Prefix + Name).str();
+  return (Prefix + Name + "." + Twine(FuncHash)).str();
 }
 
 static inline bool shouldRecordFunctionAddr(Function *F) {
   // Check the linkage
   if (!F->hasLinkOnceLinkage() && !F->hasLocalLinkage() &&
       !F->hasAvailableExternallyLinkage())
     return true;
   // Prohibit function address recording if the function is both internal and

lib/Transforms/Instrumentation/PGOInstrumentation.cpp

 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,
+    "do-comdat-renaming", cl::init(false), 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> PGOWarnMissing("pgo-warn-missing-function",
                                      cl::init(false),
                                      cl::Hidden);
FunctionHash = (uint64_t)SIVisitor.getNumOfSelectInsts() << 56 |
                  (uint64_t)IndirectCallSites.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())
+  if (!canRenameComdatInIRPGO(F))
     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.

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: $foo = comdat any
 
 ; CHECK: $__llvm_profile_raw_version = comdat any
-; CHECK: $__profv__stdin__foo.[[FOO_HASH]] = comdat any
+; CHECK: $__profv__stdin__foo.[[FOO_HASH:[0-9]+]] = comdat any
 
 @bar = global i32 ()* @foo, align 8
 
 ; CHECK: @__llvm_profile_raw_version = constant i64 {{[0-9]+}}, comdat
-; CHECK: @__profn__stdin__foo.[[FOO_HASH]] = private constant [23 x i8] c"<stdin>:foo.[[FOO_HASH]]"
+; CHECK: @__profn__stdin__foo = private constant [11 x i8] c"<stdin>:foo"
 ; 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: @__profd__stdin__foo.[[FOO_HASH]] = private global { i64, i64, i64*, i8*, i8*, i32, [1 x i16] } { i64 -5640069336071256030, 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: , 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
 }

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
+; RUN: opt < %s -mtriple=x86_64-unknown-linux -pgo-instr-gen -do-comdat-renaming=true -S | FileCheck --check-prefixes COMMON,ELFONLY %s
+; RUN: opt < %s -mtriple=x86_64-unknown-linux -passes=pgo-instr-gen -do-comdat-renaming=true -S | FileCheck --check-prefixes COMMON,ELFONLY %s
+; RUN: opt < %s -mtriple=x86_64-pc-win32-coff -pgo-instr-gen -do-comdat-renaming=true -S | FileCheck --check-prefixes COMMON,COFFONLY %s
+; RUN: opt < %s -mtriple=x86_64-pc-win32-coff -passes=pgo-instr-gen -do-comdat-renaming=true -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
 }
 

test/Transforms/PGOProfile/indirect_call_profile.ll

 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.[[FOO3_HASH:[0-9]+]] = linkonce_odr{{.*}}@foo3.[[FOO3_HASH]]
+; LOWER: @__profd_foo3.[[FOO3_HASH:[0-9]+]] = linkonce_odr{{.*}}@__profc_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()