[PATCH] D36963: Use sorting instead of a lot of stable_partitions for `ICF::segregate` function

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/COFF/ICF.cpp:212-215
+  if (A->getContents() != B->getContents())
+    return std::lexicographical_compare(
+        A->getContents().begin(), A->getContents().end(),
+        B->getContents().begin(), B->getContents().end());
----------------
ruiu wrote:
> This is probably inefficient because it may scan the same vector twice, once for `operator!=` and once for `std::lexicographical_compare`. One way of avoiding it is this.
> 
>   if (int X =
>           toStringRef(A->getContents()).compare(toStringRef(B->getContents())))
>     return X;
> 
> It is unfortuante that ArrayRef doesn't have `compare`.
Looks like 

  if (int X =
          toStringRef(A->getContents()).compare(toStringRef(B->getContents())))
    return X;

is better as it is just three lines.


================
Comment at: lld/COFF/ICF.cpp:112
+  for (size_t Pos = Begin + 1; Pos < End; Pos++) {
+    if (Equal(Chunks[GroupBegin], Chunks[Pos]))
+      continue;
----------------
I believe you can use Less instead of Equal and remove equals{Constant,Variables} functions. What am I missing?


================
Comment at: lld/COFF/ICF.cpp:115
+
+    // We use curPos as an equivalence class ID because every group ends with a
+    // unique index.
----------------
It is no longer `curPro`


================
Comment at: lld/COFF/ICF.cpp:219-222
+    // We cannot just return D1 < D2 because it will lead to non-deterministic
+    // builds
+    // so just make them equal for the purposes of sorting and separate them
+    // later
----------------
I think this comment makes sense only when you know that you previously attempted D1<D2. Since it's gone from the code, I'd remove this comment.


https://reviews.llvm.org/D36963