[PATCH] D88830: [LLD][ELF] Improve ICF for relocations to ineligible sections via "aliases"

[Andrew Ng via Phabricator via llvm-commits]

andrewng added inline comments.


================
Comment at: lld/ELF/ICF.cpp:226
+    // Add this to eqClassBase to avoid equality with unique IDs.
+    uint32_t eqClass = eqClassBase + mid;
     for (size_t i = begin; i < mid; ++i)
----------------
MaskRay wrote:
> How about making uniqueId in the main function start from `inputSections.size()`. The variable `eqClassBase` can be avoided. 
> 
> There is a gap between used values but that is not a problem: `[0, sections.size()) ... [inputSections.size(), inputSections.size()+sections.size())`
I did consider this, but I thought it would be better to not have the gap, particularly as `eqClass` is "only" 32-bit. Also having `unqueId` start from a higher value, brings it closer to hash collisions given the current code which sets the MSB to avoid such collisions. It's unlikely that this is going to make a practical difference, but it just feels better. What are your thoughts?

Whilst on this topic, do you think we should add some checks for these "range" constraints (in separate patch)?


================
Comment at: lld/test/ELF/icf-ineligible.s:1
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
----------------
MaskRay wrote:
> You can delete icf-non-mergeable.s by adding some lines to this file.
I actually thought that keeping the tests separate would be better, as in the `icf-non-mergable.s` test no sections are merged where as this test is expecting all sections to merge. But if you want me to combine the tests, I can do that. If so, which test name would be your preference?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88830/new/

https://reviews.llvm.org/D88830