[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:227
     for (size_t i = begin; i < mid; ++i)
-      sections[i]->eqClass[next] = mid;
+      sections[i]->eqClass[next] = eqClass;
 
----------------
MaskRay wrote:
> MaskRay wrote:
> > Here this actually computes non-hash IDs. Clearing the MSB (the original code) is better.
> hmm. the idea is to make this distinct from a uniqueID.
> 
> Let `uniqueId` start from inputSections.size()?
Yes, the intention is to make the equivalence class IDs distinct from the unique IDs, otherwise it might be possible (although very unlikely) for incorrect equality to occur.

I have come up with an alternative solution that uses a base offset derived from `uniqueId`.


================
Comment at: lld/ELF/ICF.cpp:358
 
     // Ineligible sections are in the special equivalence class 0.
     // They can never be the same in terms of the equivalence class.
----------------
MaskRay wrote:
> This comment is stale. This code may be deletable.
I have updated the comment, as I believe the code is still applicable.


================
Comment at: lld/ELF/ICF.cpp:476
     part.ehFrame->iterateFDEWithLSDA<ELFT>(
-        [&](InputSection &s) { s.eqClass[0] = ++uniqueId; });
+        [&](InputSection &s) { s.eqClass[0] = s.eqClass[1] = ++uniqueId; });
 
----------------
MaskRay wrote:
> Same comment: this is unneeded. combineRelocHashes updates eqClass[1]
Same reply: `combineRelocHashes()` only updates the sections that are in the vector `sections`. The sections that are assigned a unique ID here will not be added to the `sections` vector, as they are considered ineligible. Therefore, `eqClass[1]` should also be set to the same unique ID.


================
Comment at: lld/test/ELF/icf18.s:1
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t.o
----------------
MaskRay wrote:
> The test name can be improved. Consider merging this test with icf-non-mergeable.s
Had a look at `icf-non-mergeable.s` which checks for no merging, so decided just to rename to `icf-ineligible.s`. Hope that's OK.


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

https://reviews.llvm.org/D88830