[PATCH] D17529: ELF: Implement ICF.

[Sean Silva via llvm-commits]

silvas added inline comments.

================
Comment at: ELF/ICF.cpp:169
@@ +168,3 @@
+template <class ELFT>
+bool ICF<ELFT>::partition(InputSection<ELFT> **Begin, InputSection<ELFT> **End,
+                          Comparator Eq) {
----------------
The naming of this function is confusing. The name 'partition' makes it sound like it is algorithmically similar to std::partition, but it doesn't do the same thing. I would maybe call it 'segregate'.


================
Comment at: ELF/ICF.cpp:173
@@ +172,3 @@
+  InputSection<ELFT> **I = Begin;
+  for (;;) {
+    InputSection<ELFT> *Head = *I;
----------------
Please add a comment about the loop invariant. Currently it seems to be
- No element of [I,End) compares equal via Eq to any element in [Begin,I).
- If two elements of [Begin,I) compare equal via Eq, then they are part of a contiguous run of elements that all pairwise compare equal via Eq.

Also, this function is quadratic in the number of distinct elements (as determined by Eq). It is probably worth a comment explaining this and why pathological cases are not expected to happen. Without a stronger structure than Eq (e.g. a < ordering) I don't think it is actually algorithmically possible to make this non-quadratic though (since when Eq returns false you don't gain any information that can be transitively used.).

Note that in `ICF<ELFT>::run` the patch does a *sort* based on the hash (i.e. uses a stronger < structure). I believe the overall algorithm is actually correct without the initial hash assignment/sorting; it will just be quadratic in the size of the input instead of the maximum equivalence class size after the initial sort.

================
Comment at: ELF/ICF.cpp:205
@@ +204,3 @@
+template <class RelTy>
+bool ICF<ELFT>::constantEq(iterator_range<const RelTy *> RelsA,
+                           iterator_range<const RelTy *> RelsB) {
----------------
relocationsEq is probably a better name. Otherwise we have a function equalsConstant calling a function called constantEq which is confusing.


http://reviews.llvm.org/D17529