[PATCH] D79772: Nondeterminism of iterators causes false ThinLTO cache misses

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini accepted this revision.
mehdi_amini added a comment.
This revision is now accepted and ready to land.

LGTM

FYI with respect to:

> we notice that the hash value that is being used by a DenseMap is a pointer:
> 
>> static unsigned getHashValue(ValueInfo I) { return (uintptr_t)I.getRef(); }
> 
> We cannot guarantee that the compiler will allocate memory at the same
>  address for the object when we run the executable for the second time.

DenseMap is not an ordered container, we can't rely on its ordering regardless of the hash function. There is even a build-time option (`-DLLVM_REVERSE_ITERATION=ON`) to reverse the iteration order of such container to ensure we don't accidentally depend on it: https://github.com/llvm/llvm-project/blob/master/llvm/include/llvm/Support/ReverseIteration.h



================
Comment at: llvm/lib/LTO/LTO.cpp:150
+
+  // Sort the export list elemets GUIDs.
+  std::stable_sort(ExportsGUID.begin(), ExportsGUID.end());
----------------
Typo: "elemets"


================
Comment at: llvm/lib/LTO/LTO.cpp:151
+  // Sort the export list elemets GUIDs.
+  std::stable_sort(ExportsGUID.begin(), ExportsGUID.end());
+  for (uint64_t GUID : ExportsGUID) {
----------------
I think you can just run `llvm::sort(ExportsGUID);`? We don't need a "stable sort" here (can't tell the difference anyway right?)


================
Comment at: llvm/lib/LTO/LTO.cpp:160
   // sensitive to link order, so include that as well.
+  std::vector<StringRef> ImportModulesVector;
   for (auto &Entry : ImportList) {
----------------
Can you reserve here?


================
Comment at: llvm/lib/LTO/LTO.cpp:165
+  }
+  std::sort(ImportModulesVector.begin(), ImportModulesVector.end());
+
----------------
I think is can just be `sort(ImportModulesVector);` here? (llvm::sort)


================
Comment at: llvm/lib/LTO/LTO.cpp:168
+  for (StringRef ModId : ImportModulesVector) {
+    auto EntryIt = ImportList.find(ModId);
+    assert(EntryIt != ImportList.end() &&
----------------
What about storing in the vector a reference to the entry in the first place and avoid the extra lookup here?

```
 using MapEntryTy = FunctionImporter::ImportMapTy::MapEntryTy;
 std::vector<MapEntryTy *> ImportModulesVector;
 ImportModulesVector.reserve(ImportList.size());
 for (auto &Entry : ImportList) ImportModulesVector.push_back(&Entry);
 llvm::sort(ImportModulesVector, [] (const MapEntryTy *Lhs, const MapEntryTy *Rhs) {
    return Lhs < Rhs;
 });
 for (MapEntryTy *Entry : ImportModulesVector) {
   ...
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79772/new/

https://reviews.llvm.org/D79772