[PATCH] D97128: [ThinLTO] Make cloneUsedGlobalVariables deterministic

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/IR/Module.cpp:662
+GlobalVariable *llvm::collectUsedGlobalVariables(
+    const Module &M, SmallVectorImpl<GlobalValue *> &Set, bool CompilerUsed) {
+  const char *Name = CompilerUsed ? "llvm.compiler.used" : "llvm.used";
----------------
tejohnson wrote:
> Instead of having a second very similar interface, which could get confusing, how about just change it to use a set with a deterministic iterator, like std::set? Or if it is just that one callsite that needs the deterministic ordering, perhaps just copy the SmallPtrSet into a std::set there. Otherwise, the interfaces need to be carefully documented to avoid confusion. I'd lean towards just coping into a std::set in this instance for simplicity. WDYT?
We probably should delete the SmallPtrSet overload. I've checked several other call sites - The call sites don't seem to benefit from a SmallPtrSet as well.  Some call sites actually have similar non-deterministic issues. (I have confirmed that if I add a `std::reverse` I can get different bitcode files.) 

Using `std::set` will require a comparator on name. Let's use SmallVectorImpl here and migrate SmallPtrSet separately?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97128