[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:96
 void ICF::segregate(size_t Begin, size_t End, bool Constant) {
-  while (Begin < End) {
-    // Divide [Begin, End) into two. Let Mid be the start index of the
-    // second group.
-    auto Bound = std::stable_partition(
-        Chunks.begin() + Begin + 1, Chunks.begin() + End, [&](SectionChunk *S) {
-          if (Constant)
-            return equalsConstant(Chunks[Begin], S);
-          return equalsVariable(Chunks[Begin], S);
-        });
-    size_t Mid = Bound - Chunks.begin();
-
-    // Split [Begin, End) into [Begin, Mid) and [Mid, End). We use Mid as an
-    // equivalence class ID because every group ends with a unique index.
-    for (size_t I = Begin; I < Mid; ++I)
-      Chunks[I]->Class[(Cnt + 1) % 2] = Mid;
-
-    // If we created a group, we need to iterate the main loop again.
-    if (Mid != End)
-      Repeat = true;
 
+  if (Begin == End)
----------------
No need to add a blank line at beginning of a function.


================
Comment at: lld/COFF/ICF.cpp:112
+
+  // Group equal chunks together and then split [Begin, End) range into smaller ranges with equal values
+  std::stable_sort(Chunks.begin() + Begin, Chunks.begin() + End, Less);
----------------
80 cols.


================
Comment at: lld/COFF/ICF.cpp:115
+
+  size_t CurGroupBegin = Begin;
+  for (size_t CurPos = Begin + 1; CurPos < End; CurPos++) {
----------------
Since we only maintain beginning of the current group, drop `Cur`.


================
Comment at: lld/COFF/ICF.cpp:116
+  size_t CurGroupBegin = Begin;
+  for (size_t CurPos = Begin + 1; CurPos < End; CurPos++) {
+    if (Equal(Chunks[CurGroupBegin], Chunks[CurPos]))
----------------
Ditto -- CurPos -> Pos


================
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());
----------------
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`.


https://reviews.llvm.org/D36963