This is an archive of the discontinued LLVM Phabricator instance.

Utility functions for appending to llvm.used/llvm.compiler.used
ClosedPublic

Authored by eugenis on Oct 25 2016, 2:19 PM.

Details

Reviewers
pcc
mehdi_amini

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 75794.Oct 25 2016, 2:19 PM
eugenis retitled this revision from to Utility functions for appending to llvm.used/llvm.compiler.used.
eugenis updated this object.
eugenis added a reviewer: pcc.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: llvm-commits.
pcc added inline comments.Oct 25 2016, 2:38 PM
lib/LTO/LTOCodeGenerator.cpp
368

This could be a std::vector.

lib/LTO/UpdateCompilerUsed.cpp
55

This could also be a std::vector, then there'd be no need to rebuild it.

mehdi_amini added inline comments.Oct 25 2016, 2:45 PM
lib/LTO/UpdateCompilerUsed.cpp
55

How do you prevent duplicate?

pcc added inline comments.Oct 25 2016, 2:52 PM
lib/LTO/UpdateCompilerUsed.cpp
55

findLibCallsAndAsm is the only function that adds globals to LLVMUsed. The only call sites are in findInModule which cannot pass duplicates. We can easily change findLibCallsAndAsm to add the global at most once.

mehdi_amini added inline comments.Oct 25 2016, 3:06 PM
lib/LTO/UpdateCompilerUsed.cpp
55

But this is "updating" the list, so even if findLibCallsAndAsm adds the globals at most once, they can already be in the list, what did I miss?

eugenis updated this revision to Diff 75802.Oct 25 2016, 3:06 PM
eugenis added inline comments.
lib/LTO/UpdateCompilerUsed.cpp
55

The only user start with an empty list.

mehdi_amini added inline comments.Oct 25 2016, 3:10 PM
lib/LTO/UpdateCompilerUsed.cpp
55

OK I see, because of the way you split it, but see below...

lib/Transforms/Utils/ModuleUtils.cpp
104

What if some values that you add here are already in the global variable initializer?

eugenis added inline comments.Oct 25 2016, 3:22 PM
lib/Transforms/Utils/ModuleUtils.cpp
104

We'll end up with duplicates. This is how the current code works.

Btw, is not SmallPtrSet unstable in the "large mode", when it is backed by hash table? Do we not care about the order of llvm.used entries being stable?

eugenis updated this revision to Diff 75806.Oct 25 2016, 3:42 PM
eugenis marked an inline comment as done.
pcc accepted this revision.Oct 25 2016, 3:49 PM
pcc edited edge metadata.

LGTM

This revision is now accepted and ready to land.Oct 25 2016, 3:49 PM
mehdi_amini requested changes to this revision.Oct 25 2016, 3:53 PM
mehdi_amini added a reviewer: mehdi_amini.
mehdi_amini added inline comments.
lib/LTO/UpdateCompilerUsed.cpp
117

This is a set, which ensure no duplicate.

lib/Transforms/Utils/ModuleUtils.cpp
104

I don't believe it is how the current code work. I'll point it inline.

This revision now requires changes to proceed.Oct 25 2016, 3:53 PM
eugenis added inline comments.Oct 25 2016, 3:56 PM
lib/LTO/UpdateCompilerUsed.cpp
117

Now I may be missing something, but this set is converted to a vector before the current llvm.compiler.used is read. There is now deduplication between the current entries of llvm.compiler.used and the new elements. And, as Peter explained above, the new elements are unique among themselves.

mehdi_amini added inline comments.Oct 25 2016, 4:04 PM
lib/LTO/UpdateCompilerUsed.cpp
117

You're right. However it is a bug I introduced when refactoring this in http://reviews.llvm.org/D19000 ; the original code was turned the set into a vector only after appending the existing elements. So we should fix this bug and not simplify the code assuming the buggy behavior is expected.
I don't expect this API, updateCompilerUsed, to introduce a duplicate here.

eugenis updated this revision to Diff 75815.Oct 25 2016, 4:46 PM
eugenis edited edge metadata.

PTAL

mehdi_amini accepted this revision.Oct 25 2016, 4:49 PM
mehdi_amini edited edge metadata.

Thanks.

This revision is now accepted and ready to land.Oct 25 2016, 4:49 PM
eugenis closed this revision.Oct 25 2016, 5:03 PM

r285143
Thanks for the review.