Skip to content

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
@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