Skip to content

[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

Merged
merged 2 commits into from
May 19, 2024

Conversation

mingmingl-llvm
Copy link
Contributor

@mingmingl-llvm mingmingl-llvm commented May 18, 2024

@MaskRay points out std:: doesn't randomize the iteration order of unordered_{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)

@llvmbot llvmbot added the LTO Link time optimization (regular/full LTO or ThinLTO) label May 18, 2024
@llvmbot
Copy link
Member

llvmbot commented May 18, 2024

@llvm/pr-subscribers-lto

Author: Mingming Liu (minglotus-6)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/92622.diff

1 Files Affected:

  • (modified) llvm/lib/LTO/LTO.cpp (+9-1)
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.

Copy link
Member

@MaskRay MaskRay left a 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++...)

ImportedGUIDs.push_back(Fn);

llvm::sort(ImportedGUIDs);

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the pointers!

@mingmingl-llvm
Copy link
Contributor Author

Test failure is irrelevant.

cat github-pull-requests_build_65187_linux-linux-x64.log | grep -B 5 -A 10 "Failed Tests"
[0.20s,0.30s) :: [***********                             ] :: [120/418]
[0.10s,0.20s) :: [******************                      ] :: [191/418]
[0.00s,0.10s) :: [*******                                 ] :: [ 74/418]
--------------------------------------------------------------------------
********************
Failed Tests (1):
  BOLT :: RISCV/unnamed-sym-no-entry.c


Testing Time: 2.47s

Total Discovered Tests: 431
  Skipped          :   7 (1.62%)
  Unsupported      :  13 (3.02%)
  Passed           : 409 (94.90%)
  Expectedly Failed:   1 (0.23%)

@mingmingl-llvm mingmingl-llvm merged commit d34be64 into llvm:main May 19, 2024
3 of 4 checks passed
@mingmingl-llvm mingmingl-llvm deleted the sort branch May 19, 2024 02:40
qiaojbao pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Jun 5, 2024
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LTO Link time optimization (regular/full LTO or ThinLTO)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants