-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[MemProf] Suppress duplicate clones in the LTO backend #161551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -29,6 +29,7 @@ | |||||||
#include "llvm/ADT/SmallSet.h" | ||||||||
#include "llvm/ADT/SmallVector.h" | ||||||||
#include "llvm/ADT/Statistic.h" | ||||||||
#include "llvm/ADT/StringExtras.h" | ||||||||
#include "llvm/Analysis/MemoryProfileInfo.h" | ||||||||
#include "llvm/Analysis/ModuleSummaryAnalysis.h" | ||||||||
#include "llvm/Analysis/OptimizationRemarkEmitter.h" | ||||||||
|
@@ -40,6 +41,7 @@ | |||||||
#include "llvm/Support/CommandLine.h" | ||||||||
#include "llvm/Support/GraphWriter.h" | ||||||||
#include "llvm/Support/InterleavedRange.h" | ||||||||
#include "llvm/Support/SHA1.h" | ||||||||
#include "llvm/Support/raw_ostream.h" | ||||||||
#include "llvm/Transforms/IPO.h" | ||||||||
#include "llvm/Transforms/Utils/CallPromotionUtils.h" | ||||||||
|
@@ -60,6 +62,9 @@ STATISTIC(FunctionClonesThinBackend, | |||||||
"Number of function clones created during ThinLTO backend"); | ||||||||
STATISTIC(FunctionsClonedThinBackend, | ||||||||
"Number of functions that had clones created during ThinLTO backend"); | ||||||||
STATISTIC( | ||||||||
FunctionCloneDuplicatesThinBackend, | ||||||||
"Number of function clone duplicates detected during ThinLTO backend"); | ||||||||
STATISTIC(AllocTypeNotCold, "Number of not cold static allocations (possibly " | ||||||||
"cloned) during whole program analysis"); | ||||||||
STATISTIC(AllocTypeCold, "Number of cold static allocations (possibly cloned) " | ||||||||
|
@@ -5186,19 +5191,129 @@ bool CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::assignFunctions() { | |||||||
return Changed; | ||||||||
} | ||||||||
|
||||||||
// Compute a SHA1 hash of the callsite and alloc version information of clone I | ||||||||
// in the summary, to use in detection of duplicate clones. | ||||||||
std::string ComputeHash(StringMap<Function *> &HashToFunc, FunctionSummary *FS, | ||||||||
unsigned I) { | ||||||||
SHA1 Hasher; | ||||||||
// Update hash with any callsites that call non-default (non-zero) callee | ||||||||
// versions. | ||||||||
for (auto &SN : FS->callsites()) { | ||||||||
// In theory all callsites and allocs in this function should have the same | ||||||||
// number of clone entries, but handle any discrepancies gracefully below | ||||||||
// for NDEBUG builds. | ||||||||
assert( | ||||||||
SN.Clones.size() > I && | ||||||||
"Callsite summary has fewer entries than other summaries in function"); | ||||||||
if (SN.Clones.size() <= I || !SN.Clones[I]) | ||||||||
continue; | ||||||||
uint8_t Data[4]; | ||||||||
|
uint8_t Data[4]; | |
uint8_t Data[sizeof(SN.Clones[I])]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are OK with returning uint64_t
instead, may I suggest the following?:
return toHex(Hasher.result()); | |
return support::endian::read64le(Hasher.result().data()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This lambda and the one below were refactored out of existing code to enable reuse
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid confusion with Hash
declared in the for
loop below, may I suggest the following? I don't think you use this Hash
later in this function.
auto Hash = ComputeHash(HashToFunc, FS, 0); | |
HashToFunc[Hash] = &F; | |
HashToFunc[ComputeHash(HashToFunc, FS, 0)] = &F; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this small block of code warrants a comment:
// If the VMap is empty, this clone was a duplicate of another and was created
// as an alias or a declaration.
The same applies to the three more instances of if
statements below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I suggest
const FunctionSummary *FS
?Now, for the return value, I'm wondering if
uint64_t
is sufficient instead of the full 160-bit SHA1. If you are OK withuint64_t
, see the comment at the return statement below.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea on return type! done