[llvm] r291588 - [PGO] Turn off comdat renaming in IR PGO by default
Rong Xu via llvm-commits
llvm-commits at lists.llvm.org
Tue Jan 10 11:30:21 PST 2017
Author: xur
Date: Tue Jan 10 13:30:20 2017
New Revision: 291588
URL: http://llvm.org/viewvc/llvm-project?rev=291588&view=rev
Log:
[PGO] Turn off comdat renaming in IR PGO by default
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.
Reviewers: davidxl
Subscribers: llvm-commits, xur
Differential Revision: https://reviews.llvm.org/D28416
Added:
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=291588&r1=291587&r2=291588&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
+++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Tue Jan 10 13:30:20 2017
@@ -230,6 +230,15 @@ 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=291588&r1=291587&r2=291588&view=diff
==============================================================================
--- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
+++ llvm/trunk/lib/ProfileData/InstrProf.cpp Tue Jan 10 13:30:20 2017
@@ -811,4 +811,47 @@ 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=291588&r1=291587&r2=291588&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Instrumentation/InstrProfiling.cpp (original)
+++ llvm/trunk/lib/Transforms/Instrumentation/InstrProfiling.cpp Tue Jan 10 13:30:20 2017
@@ -32,6 +32,11 @@ 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"),
@@ -272,7 +277,16 @@ void InstrProfiling::lowerCoverageData(G
static std::string getVarName(InstrProfIncrementInst *Inc, StringRef Prefix) {
StringRef NamePrefix = getInstrProfNameVarPrefix();
StringRef Name = Inc->getName()->getName().substr(NamePrefix.size());
- return (Prefix + Name).str();
+ 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();
}
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=291588&r1=291587&r2=291588&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp (original)
+++ llvm/trunk/lib/Transforms/Instrumentation/PGOInstrumentation.cpp Tue Jan 10 13:30:20 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(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"));
@@ -407,20 +407,8 @@ void FuncPGOInstrumentation<Edge, BBInfo
static bool canRenameComdat(
Function &F,
std::unordered_multimap<Comdat *, GlobalValue *> &ComdatMembers) {
- if (F.getName().empty())
+ if (!DoComdatRenaming || !canRenameComdatFunc(F, true))
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.
Added: 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=291588&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext (added)
+++ llvm/trunk/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext Tue Jan 10 13:30:20 2017
@@ -0,0 +1,36 @@
+# 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=291588&r1=291587&r2=291588&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/PGOProfile/comdat_internal.ll (original)
+++ llvm/trunk/test/Transforms/PGOProfile/comdat_internal.ll Tue Jan 10 13:30:20 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.[[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
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=291588&r1=291587&r2=291588&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/PGOProfile/comdat_rename.ll (original)
+++ llvm/trunk/test/Transforms/PGOProfile/comdat_rename.ll Tue Jan 10 13:30:20 2017
@@ -1,7 +1,7 @@
-; 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
@@ -38,22 +38,10 @@ 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=291588&r1=291587&r2=291588&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/PGOProfile/indirect_call_profile.ll (original)
+++ llvm/trunk/test/Transforms/PGOProfile/indirect_call_profile.ll Tue Jan 10 13:30:20 2017
@@ -54,7 +54,7 @@ bb11:
}
; 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
Added: 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=291588&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/PGOProfile/multiple_hash_profile.ll (added)
+++ llvm/trunk/test/Transforms/PGOProfile/multiple_hash_profile.ll Tue Jan 10 13:30:20 2017
@@ -0,0 +1,36 @@
+; 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