From: Rong Xu Date: Tue, 10 Jan 2017 19:30:20 +0000 (+0000) Subject: [PGO] Turn off comdat renaming in IR PGO by default X-Git-Tag: android-x86-7.1-r4~22240 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=2cf892fefa486af61f51f2ed7800e6314c2ffd0b;p=android-x86%2Fexternal-llvm.git [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 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@291588 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/llvm/ProfileData/InstrProf.h b/include/llvm/ProfileData/InstrProf.h index 094f3af005d..c7e558efa3d 100644 --- a/include/llvm/ProfileData/InstrProf.h +++ b/include/llvm/ProfileData/InstrProf.h @@ -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" diff --git a/lib/ProfileData/InstrProf.cpp b/lib/ProfileData/InstrProf.cpp index 77c6ffc9c25..74acd9e5e20 100644 --- a/lib/ProfileData/InstrProf.cpp +++ b/lib/ProfileData/InstrProf.cpp @@ -811,4 +811,47 @@ 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(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/lib/Transforms/Instrumentation/InstrProfiling.cpp b/lib/Transforms/Instrumentation/InstrProfiling.cpp index 8da3e31200f..adea7e77244 100644 --- a/lib/Transforms/Instrumentation/InstrProfiling.cpp +++ b/lib/Transforms/Instrumentation/InstrProfiling.cpp @@ -32,6 +32,11 @@ cl::opt DoNameCompression("enable-name-compression", cl::desc("Enable name string compression"), cl::init(true)); +cl::opt DoHashBasedCounterSplit( + "hash-based-counter-split", + cl::desc("Rename counter variable of a comdat function based on cfg hash"), + cl::init(true)); + cl::opt ValueProfileStaticAlloc( "vp-static-alloc", cl::desc("Do static counter allocation for value profiler"), @@ -272,7 +277,16 @@ void InstrProfiling::lowerCoverageData(GlobalVariable *CoverageNamesVar) { 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 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) { diff --git a/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/lib/Transforms/Instrumentation/PGOInstrumentation.cpp index 28f4f7ea145..ffebd42795d 100644 --- a/lib/Transforms/Instrumentation/PGOInstrumentation.cpp +++ b/lib/Transforms/Instrumentation/PGOInstrumentation.cpp @@ -119,7 +119,7 @@ static cl::opt 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 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::computeCFGHash() { static bool canRenameComdat( Function &F, std::unordered_multimap &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. diff --git a/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext b/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext new file mode 100644 index 00000000000..5bf67fb2bfa --- /dev/null +++ b/test/Transforms/PGOProfile/Inputs/multiple_hash_profile.proftext @@ -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 + diff --git a/test/Transforms/PGOProfile/comdat_internal.ll b/test/Transforms/PGOProfile/comdat_internal.ll index 25dafbea103..7df6f91fe72 100644 --- a/test/Transforms/PGOProfile/comdat_internal.ll +++ b/test/Transforms/PGOProfile/comdat_internal.ll @@ -4,17 +4,17 @@ 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":foo.[[FOO_HASH]]" +; CHECK: @__profn__stdin__foo = private constant [11 x i8] c":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 diff --git a/test/Transforms/PGOProfile/comdat_rename.ll b/test/Transforms/PGOProfile/comdat_rename.ll index b69c802093b..eb9ddb4a1ce 100644 --- a/test/Transforms/PGOProfile/comdat_rename.ll +++ b/test/Transforms/PGOProfile/comdat_rename.ll @@ -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 -@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() { diff --git a/test/Transforms/PGOProfile/indirect_call_profile.ll b/test/Transforms/PGOProfile/indirect_call_profile.ll index 409c29ef872..e1f499c08a7 100644 --- a/test/Transforms/PGOProfile/indirect_call_profile.ll +++ b/test/Transforms/PGOProfile/indirect_call_profile.ll @@ -54,7 +54,7 @@ bb11: ; preds = %bb2 } ; 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 diff --git a/test/Transforms/PGOProfile/multiple_hash_profile.ll b/test/Transforms/PGOProfile/multiple_hash_profile.ll new file mode 100644 index 00000000000..f4041830f8f --- /dev/null +++ b/test/Transforms/PGOProfile/multiple_hash_profile.ll @@ -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 + +@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 +} + +