[PATCH] D97128: [ThinLTO] Make cloneUsedGlobalVariables deterministic
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Feb 21 00:14:39 PST 2021
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
More information about the llvm-commits
mailing list