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

Rong Xu via llvm-commits llvm-commits at lists.llvm.org
Tue Jan 10 15:54:32 PST 2017


Author: xur
Date: Tue Jan 10 17:54:31 2017
New Revision: 291621

URL: http://llvm.org/viewvc/llvm-project?rev=291621&view=rev
Log:
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.

Removed:
    llvm/trunk/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext
    llvm/trunk/test/Transforms/PGOProfile/multiple_hash_profile.ll
Modified:
    llvm/trunk/include/llvm/ProfileData/InstrProf.h
    llvm/trunk/lib/ProfileData/InstrProf.cpp
    llvm/trunk/lib/Transforms/Instrumentation/InstrProfiling.cpp
    llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
    llvm/trunk/test/Transforms/PGOProfile/comdat_internal.ll
    llvm/trunk/test/Transforms/PGOProfile/comdat_rename.ll
    llvm/trunk/test/Transforms/PGOProfile/indirect_call_profile.ll

Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=291621&r1=291620&r2=291621&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
+++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Tue Jan 10 17:54:31 2017
@@ -230,15 +230,6 @@ class InstrProfSymtab;
 /// 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. Instances of the same
-/// comdat function may have different control flows thus can not share the
-/// same counter variable.
-bool canRenameComdatFunc(const Function &F, bool CheckAddressTaken = false);
-
 enum InstrProfValueKind : uint32_t {
 #define VALUE_PROF_KIND(Enumerator, Value) Enumerator = Value,
 #include "llvm/ProfileData/InstrProfData.inc"

Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=291621&r1=291620&r2=291621&view=diff
==============================================================================
--- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
+++ llvm/trunk/lib/ProfileData/InstrProf.cpp Tue Jan 10 17:54:31 2017
@@ -811,47 +811,4 @@ bool needsComdatForCounter(const Functio
 
   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

Modified: llvm/trunk/lib/Transforms/Instrumentation/InstrProfiling.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/InstrProfiling.cpp?rev=291621&r1=291620&r2=291621&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Instrumentation/InstrProfiling.cpp (original)
+++ llvm/trunk/lib/Transforms/Instrumentation/InstrProfiling.cpp Tue Jan 10 17:54:31 2017
@@ -32,11 +32,6 @@ cl::opt<bool> DoNameCompression("enable-
                                 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(G
 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) {

Modified: llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp?rev=291621&r1=291620&r2=291621&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp (original)
+++ llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp Tue Jan 10 17:54:31 2017
@@ -119,7 +119,7 @@ static cl::opt<unsigned> MaxNumAnnotatio
 // 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
 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.

Removed: llvm/trunk/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext?rev=291620&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext (original)
+++ llvm/trunk/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext (removed)
@@ -1,36 +0,0 @@
-# IR level Instrumentation Flag
-:ir
-_Z3fooi
-# Func Hash:
-72057606922829823
-# Num Counters:
-2
-# Counter Values:
-18
-12
-
-_Z3fooi
-# Func Hash:
-12884901887
-# Num Counters:
-1
-# Counter Values:
-0
-
-_Z3bari
-# Func Hash:
-72057606922829823
-# Num Counters:
-2
-# Counter Values:
-0
-0
-
-_Z4m2f1v
-# Func Hash:
-12884901887
-# Num Counters:
-1
-# Counter Values:
-1
-

Modified: llvm/trunk/test/Transforms/PGOProfile/comdat_internal.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/PGOProfile/comdat_internal.ll?rev=291621&r1=291620&r2=291621&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/PGOProfile/comdat_internal.ll (original)
+++ llvm/trunk/test/Transforms/PGOProfile/comdat_internal.ll Tue Jan 10 17:54:31 2017
@@ -4,17 +4,17 @@ target datalayout = "e-m:e-i64:64-f80:12
 target triple = "x86_64-unknown-linux-gnu"
 
 $foo = comdat any
-; CHECK: $foo = comdat any
+; CHECK: $foo.[[FOO_HASH:[0-9]+]] = comdat any
 
 ; CHECK: $__llvm_profile_raw_version = comdat any
-; CHECK: $__profv__stdin__foo.[[FOO_HASH:[0-9]+]] = 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: @__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 -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: @__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: , i8* null, i32 1, [1 x i16] zeroinitializer }, section "__llvm_prf_data", comdat($__profv__stdin__foo.[[FOO_HASH]]), align 8
 ; CHECK: @__llvm_prf_nm

Modified: llvm/trunk/test/Transforms/PGOProfile/comdat_rename.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/PGOProfile/comdat_rename.ll?rev=291621&r1=291620&r2=291621&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/PGOProfile/comdat_rename.ll (original)
+++ llvm/trunk/test/Transforms/PGOProfile/comdat_rename.ll Tue Jan 10 17:54:31 2017
@@ -1,7 +1,7 @@
-; 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
+; 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
@@ -38,10 +38,22 @@ 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
+ at 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
 
 ; ELFONLY: @f = weak alias void (), void ()* @f.[[SINGLEBB_HASH]]
+; ELFONLY: @f_with_alias = weak alias void (), void ()* @f_with_alias.[[SINGLEBB_HASH]]
+; ELFONLY: @af = weak alias void (...), void (...)* @af.[[SINGLEBB_HASH]]
 ; ELFONLY: @aef = weak alias void (), void ()* @aef.[[SINGLEBB_HASH]]
 
 define available_externally void @aef() {

Modified: llvm/trunk/test/Transforms/PGOProfile/indirect_call_profile.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/PGOProfile/indirect_call_profile.ll?rev=291621&r1=291620&r2=291621&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/PGOProfile/indirect_call_profile.ll (original)
+++ llvm/trunk/test/Transforms/PGOProfile/indirect_call_profile.ll Tue Jan 10 17:54:31 2017
@@ -54,7 +54,7 @@ bb11:
 }
 
 ; Test that comdat function's address is recorded.
-; LOWER: @__profd_foo3.[[FOO3_HASH:[0-9]+]] = linkonce_odr{{.*}}@__profc_foo3.[[FOO3_HASH]]
+; 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

Removed: llvm/trunk/test/Transforms/PGOProfile/multiple_hash_profile.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/PGOProfile/multiple_hash_profile.ll?rev=291620&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/PGOProfile/multiple_hash_profile.ll (original)
+++ llvm/trunk/test/Transforms/PGOProfile/multiple_hash_profile.ll (removed)
@@ -1,36 +0,0 @@
-; RUN: llvm-profdata merge %S/Inputs/multiple_hash_profile.proftext -o %t.profdata
-; RUN: opt < %s -pgo-instr-use -pgo-test-profile-file=%t.profdata  -S | FileCheck %s
-; RUN: opt < %s -passes=pgo-instr-use -pgo-test-profile-file=%t.profdata -S | FileCheck %s
-target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
-target triple = "x86_64-unknown-linux-gnu"
-
-$_Z3fooi = comdat any
-
- at g2 = local_unnamed_addr global i32 (i32)* null, align 8
-
-define i32 @_Z3bari(i32 %i) {
-entry:
-  %cmp = icmp sgt i32 %i, 2
-  %mul = select i1 %cmp, i32 1, i32 %i
-  %retval.0 = mul nsw i32 %mul, %i
-  ret i32 %retval.0
-}
-
-define void @_Z4m2f1v() {
-entry:
-  store i32 (i32)* @_Z3fooi, i32 (i32)** @g2, align 8
-  ret void
-}
-
-define linkonce_odr i32 @_Z3fooi(i32 %i) comdat {
-entry:
-  %cmp.i = icmp sgt i32 %i, 2
-  %mul.i = select i1 %cmp.i, i32 1, i32 %i
-; CHECK: %mul.i = select i1 %cmp.i, i32 1, i32 %i
-; CHECK-SAME !prof ![[BW:[0-9]+]]
-; CHECK ![[BW]] = !{!"branch_weights", i32 12, i32 6}
-  %retval.0.i = mul nsw i32 %mul.i, %i
-  ret i32 %retval.0.i
-}
-
-




More information about the llvm-commits mailing list