[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:89-91
+  if (Begin == End)
+    return;
 
----------------
I don't think this extra check is needed.


================
Comment at: lld/COFF/ICF.cpp:102-120
+  size_t GroupBegin = Begin;
+  for (size_t Pos = Begin + 1; Pos < End; Pos++) {
+    if (!Less(Chunks[GroupBegin], Chunks[Pos]))
+      continue;
+
+    // We use Pos as an equivalence class ID because every group ends with a
+    // unique index.
----------------
I wonder this code is correct. If you directly translate the original code to use Less instead of Equals, it would be something like this:

  while (Begin < End) {
    // Find a sequence of elements that belong to the same equivalence class.
    size_t I = Begin + 1;
    while (I < End && !Less(Chunks[Begin], Chunks[I]))
      ++I;

    // Split [Begin, End) into [Begin, I) and [I, End). We use I as an
    // equivalence class ID because every group ends with a unique index.
    for (size_t J = Begin; J < I; ++J)
      Chunks[J]->Class[(Cnt + 1) % 2] = I;

    // If we created a group, we need to iterate the main loop again.
    if (I != End)
      Repeat = true;
    Begin = I;
  }

I think this is a bit more readable.


================
Comment at: lld/COFF/ICF.cpp:178
+
+  return 0;
 }
----------------
return false;


================
Comment at: lld/COFF/ICF.cpp:202-203
+
+    if (D1->getChunk()->Class[Cnt % 2] != D2->getChunk()->Class[Cnt % 2])
+      return D1->getChunk()->Class[Cnt % 2] < D2->getChunk()->Class[Cnt % 2];
+  }
----------------
For consistency, I'd assign them C1 and C2 just like above.


================
Comment at: lld/COFF/ICF.cpp:206
+
+  return 0;
 }
----------------
return false;


https://reviews.llvm.org/D36963