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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

I believe you missed a point that we use symbols differently in comparison if all the others are the same. In this case, the address. Let me describe it more concretely.

Let's say we have three chunks, C1, C2 and C3 and symbols https://reviews.llvm.org/B1, https://reviews.llvm.org/B2 and https://reviews.llvm.org/B3. https://reviews.llvm.org/B1 and https://reviews.llvm.org/B3 are DefinedRegular, but https://reviews.llvm.org/B2 is not. Their addresses are https://reviews.llvm.org/B1<https://reviews.llvm.org/B2<https://reviews.llvm.org/B3. Each chunk has only one relocation, and C1 has https://reviews.llvm.org/B1 as a relocation target, C2 has https://reviews.llvm.org/B2 as a relocation target, and so on. All other chunk attributes (size, contents, alignment, etc.) are all the same.

Now consider lessVariable(C1, C2). At line 191, lessVariable returns true because https://reviews.llvm.org/B1<https://reviews.llvm.org/B2. How about lessVariable(C2, C3)? At line 191, lessVaraible returns true because https://reviews.llvm.org/B2<https://reviews.llvm.org/B3.  At this point, it concludes that C1<C2<C3. But that's wrong because both lessVaraible(C1, C3) and lessVariable(C3, C1) return false. That's what I was saying that the comparison function is not transitive.


https://reviews.llvm.org/D36963