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

[Alex Telishev via Phabricator via llvm-commits]

alex.telishev added inline comments.


================
Comment at: lld/COFF/ICF.cpp:214
+
+    return B1 < B2;
   };
----------------
ruiu wrote:
> alex.telishev wrote:
> > alex.telishev wrote:
> > > ruiu wrote:
> > > > I think this comparison breaks determinism because it compares pointer values that change on every process invocation. You may be able to just return `false`?
> > > Unfortunately, no - that would break strict ordering (`A < B == false` and `B < A == false`, like NaN) and  `stable_sort` will break in this case. It's possible to `return true` and make them all be equal to each other for the purposes of sorting. But in this case loop after `stable_sort` in `segregate` function needs to use different comparison. It's possible to use `equalsConstant` from before this patch (as we only need equality there), but it'll be a lot of copy-paste. 
> > Sorry, it's exact opposite to what I said, `stable_sort` will break if `return true` is used, and on `return false;` everything works, but we need to change comparison in `segregate`.
> I'm not sure if I understand your point correctly. If a comparison function returns false for both (A, B) and (B, A), then it means that A == B, and std::stable_sort sort them accordingly (i.e. do not alter relative order of A and B), no?
> 
> On the other hand, I believe returning true for both (A, B) and (B, A) breaks strict weak ordering, as A<B && B<A cannot be true.
Yes, you're right. But comparison after that still needs to be changed. Are you ok with using old version that do only equality comparison for this case?


================
Comment at: lld/COFF/ICF.cpp:247
+
+  return lexicographicalCompareRelocs(A, B, Less);
+}
----------------
ruiu wrote:
> It looks like this `lexicographicalCompareRelocs` function doesn't help code readability that much. You might want to just do things in-place instead of passing a callback function to another function. I'd do something like this:
> 
>   for (size_t I = 0; I != A->NumRelocs; ++I) {
>     const coff_relocation *R1 = A->Relocs.begin() + I;
>     const coff_relocation *R2 = B->Relocs.begin() + I;
> 
>     if (R1.Type != R2.Type)
>       return R1.Type < R2.Type;
>     if (R1.VirtualAddress != R2.VirtualAddress)
>       return R1.VirtualAddress < R2.VirtualAddress;
> 
>     SymbolBody *B1 = A->File->getSymbolBody(R1.SymbolTableIndex);
>     SymbolBody *B2 = B->File->getSymbolBody(R2.SymbolTableIndex);
>     if (B1 == B2)
>       continue;
> 
>     auto *D1 = dyn_cast<DefinedRegular>(B1);
>     auto *D2 = dyn_cast<DefinedRegular>(B2);
>     if (!D1 || D2)
>       continue;
>     if (D1->getValue() != D2->getValue())
>       return D1->getValue() < D2->getValue();
> 
>     uint32_t C1 = D1->getChunk()->Class[Cnt % 2];
>     uint32_t C2 = D2->getChunk()->Class[Cnt % 2];
>     if (C1 != C2)
>       return C1 < C2;
>   }
>   return false;
> 
> Note that when the control reaches this loop, A->NumReloc == B->NumReloc is guaranteed because we do have a check for that at the beginning of this function.
Agreed, that way it even will be faster, because no need to call comparison function twice.


================
Comment at: lld/COFF/ICF.cpp:112
+  for (size_t Pos = Begin + 1; Pos < End; Pos++) {
+    if (Equal(Chunks[GroupBegin], Chunks[Pos]))
+      continue;
----------------
ruiu wrote:
> I believe you can use Less instead of Equal and remove equals{Constant,Variables} functions. What am I missing?
We changed `Less{Constant,Variables}` functions so now every non-`DefinedRegular` relocations are equal to each other.  As the result they will all receive the same Class if we use `Less` function in the loop.  


https://reviews.llvm.org/D36963