-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[ThinLTO]Sort imported GUIDs before cache key update #92622
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
Conversation
@llvm/pr-subscribers-lto Author: Mingming Liu (minglotus-6) ChangesFull diff: https://github.com/llvm/llvm-project/pull/92622.diff 1 Files Affected:
diff --git a/llvm/lib/LTO/LTO.cpp b/llvm/lib/LTO/LTO.cpp
index 53060df7f503e..afc687a02d965 100644
--- a/llvm/lib/LTO/LTO.cpp
+++ b/llvm/lib/LTO/LTO.cpp
@@ -199,13 +199,21 @@ void llvm::computeLTOCacheKey(
[](const ImportModule &Lhs, const ImportModule &Rhs) -> bool {
return Lhs.getHash() < Rhs.getHash();
});
+ std::vector<uint64_t> ImportedGUIDs;
for (const ImportModule &Entry : ImportModulesVector) {
auto ModHash = Entry.getHash();
Hasher.update(ArrayRef<uint8_t>((uint8_t *)&ModHash[0], sizeof(ModHash)));
AddUint64(Entry.getFunctions().size());
+
+ ImportedGUIDs.clear();
for (auto &Fn : Entry.getFunctions())
- AddUint64(Fn);
+ ImportedGUIDs.push_back(Fn);
+
+ llvm::sort(ImportedGUIDs);
+
+ for (auto &GUID : ImportedGUIDs)
+ AddUint64(GUID);
}
// Include the hash for the resolved ODR.
|
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.
Thanks! https://abseil.io/docs/cpp/guides/container#iteration-order-instability and DenseMap/StringMap "randomize" the iteration order to catch mistakes, but libstdc++/libc++ unordered_{set,map} haven't implemented this technique. (Many simple concepts are extremely difficult to implement portably in libc++...)
llvm/lib/LTO/LTO.cpp
Outdated
ImportedGUIDs.push_back(Fn); | ||
|
||
llvm::sort(ImportedGUIDs); | ||
|
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.
There are quite a few blank lines but I think we can be more dense.
Group coherent code block of multiple lines, probably not just one/two lines.
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.
thanks for the pointers!
Test failure is irrelevant.
|
…9b6ae8b37 Local branch amd-gfx 95f9b6a Merged main:597ac471cc7da97ccf957362a7e9f7a52d6910ee into amd-gfx:e36a339769fe Remote branch main d34be64 [ThinLTO]Sort imported GUIDs before cache key update (llvm#92622)
@MaskRay points out
std::
doesn't randomize the iteration order ofunordered_{set,map}
, and the iteration order for single build is deterministic.Specifically, add 'sort' here since it's helpful when container type changes (for example, #88024 wants to change container type from
unordered_set
to `DenseMap)