[PATCH] D46755: [LLD][ELF] Implement --keep-unique option

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/ICF.cpp:439-440
     if (auto *S = dyn_cast<InputSection>(Sec))
-      if (isEligible(S))
+      if (isEligible(S) &&
+          KeepUniqueSections.find(S) == KeepUniqueSections.end())
         Sections.push_back(S);
----------------
ruiu wrote:
> I think that looking up a hash function for each input section is slow, so I'd avoid doing this. Instead, I believe you can add a `KeepUnique` bit to the InputSection and set that bit in the driver.
I think adding `KeepUnique` will not change the size of `SectionBase`, so that is probably ok. 

But at the same time, I just wonder why do you think it should be slow? I do not expect to see more than ... well, 50? sections in a set. That probably should not be slow here. We could avoid adding the flag then.


================
Comment at: test/ELF/icf-keep-unique.s:8
+// Base case, expect only .text.f1 to be kept
+// CHECK: selected section /linaro/icf/build/tools/lld/test/ELF/Output/icf-keep-unique.s.tmp:(.text.f1)
+// CHECK-NEXT:   removing identical section /linaro/icf/build/tools/lld/test/ELF/Output/icf-keep-unique.s.tmp:(.text.f2)
----------------
This contains absolute path and will fail in a different environment I think.


================
Comment at: test/ELF/icf-keep-unique.s:28
+  mov $42, %rdi
+  syscall
+
----------------
can these 3 lines be a nop?


https://reviews.llvm.org/D46755