[PATCH] D17529: ELF: Implement ICF.

[Rui Ueyama via llvm-commits]

ruiu marked 3 inline comments as done.

================
Comment at: ELF/ICF.cpp:10
@@ +9,3 @@
+//
+// Identical COMDAT Folding is a feature to merge COMDAT sections not by
+// name (which is regular COMDAT handling) but by contents. If two COMDAT
----------------
rafael wrote:
> On ELF it is possible to merge non COMDAT sections. We can keep the acronym by doing what gold does and calling this Identical *Code* Folding.
Done.

================
Comment at: ELF/ICF.cpp:118
@@ +117,3 @@
+  ELFFile<ELFT> &EObj = S->File->getObj();
+  const Elf_Shdr *RelSec = S->RelocSections[0];
+
----------------
rafael wrote:
> Assert that there only one? Return 0 if there is none?
Done.

================
Comment at: ELF/ICF.cpp:120
@@ +119,3 @@
+
+  if (RelSec->sh_type == SHT_RELA) {
+    auto Rels = EObj.relas(RelSec);
----------------
rafael wrote:
> It might be cleaner to just say RelSec->sh_size/RelSec->sh_entsize;
Done.

================
Comment at: ELF/ICF.cpp:128
@@ +127,3 @@
+
+// Returns a hash value for S. Note that relocation information is not
+// included in the hash value.
----------------
rafael wrote:
> Not exactly true since the number of relocations is included.
> 
> Since this is the only use of numRels and it is only used for hashing, how about just using RelSec->sh_size?
Done.

================
Comment at: ELF/ICF.cpp:150
@@ +149,3 @@
+  const Elf_Shdr &H = *S->getSectionHdr();
+  return H.sh_type == SHT_PROGBITS &&
+         (H.sh_flags & SHF_ALLOC) &&
----------------
rafael wrote:
> Why only PROGBITS? OK if it is just to keep it simple for now.
It doesn't have to be PROGBITS only. Removed.

================
Comment at: ELF/ICF.cpp:168
@@ +167,3 @@
+// for new groups.
+template <class ELFT>
+bool ICF<ELFT>::partition(InputSection<ELFT> **Begin, InputSection<ELFT> **End,
----------------
rafael wrote:
> Return true if a new group was created.
> 
> This could also return void and have the caller check NextId.
Done.

================
Comment at: ELF/ICF.cpp:232
@@ +231,3 @@
+    const Elf_Shdr *RB = B->RelocSections[0];
+    if (RA->sh_type != RB->sh_type)
+      return false;
----------------
rafael wrote:
> I don't think this is possible.
> 
> It is all SHT_RELA or SHT_REL.
Removed.

================
Comment at: ELF/ICF.cpp:247
@@ +246,3 @@
+         A->getSize() == B->getSize() &&
+         A->getAlign() == B->getAlign() &&
+         A->getSectionData() == B->getSectionData();
----------------
rafael wrote:
> It is possible to merge when the alignment is different, we just have to keep the larger one. A fixme for now is fine.
> 
Done.

================
Comment at: ELF/ICF.cpp:262
@@ +261,3 @@
+  if (EA - IA != EB - IB)
+    return false;
+  for (; IA != EA; ++IA, ++IB) {
----------------
rafael wrote:
> The same 'if' is in equalsContant. Do you need it here?
Removed.

================
Comment at: ELF/ICF.cpp:268
@@ +267,3 @@
+      continue;
+    if (!SA || !SB)
+      return false;
----------------
rafael wrote:
> If both are null they could still be equal, no?
> 
> FIXME for now is fine.
Yes, and in that case the above `if (SA== SB)` should be true, so it should already be covered.

================
Comment at: ELF/ICF.cpp:272
@@ +271,3 @@
+    InputSection<ELFT> *Y = dyn_cast<InputSection<ELFT>>(SB);
+    if (!X || !Y || X->GroupId != Y->GroupId)
+      return false;
----------------
rafael wrote:
> This is also conservatively correct, right?
> 
> Two relocations pointing to the string "foobar" in a SHF_MERGE section are equivalent.
I believe the above `if (SA == SB)` should cover most cases already.

================
Comment at: ELF/ICF.cpp:311
@@ +310,3 @@
+  // the same group are consecutive in the vector.
+  std::sort(V.begin(), V.end(),
+            [](InputSection<ELFT> *A, InputSection<ELFT> *B) {
----------------
rafael wrote:
> Do we need stable_sort to make this deterministic?
Done.

================
Comment at: ELF/InputSection.cpp:31
@@ +30,3 @@
+  // If GC is disabled, all sections are considered live by default.
+  Live = Config && !Config->GcSections;
+}
----------------
rafael wrote:
> When is Config null?
`Discarded` section is initialized statically, and when it is initialized, Config is still null.

================
Comment at: ELF/InputSection.cpp:75
@@ -70,3 +74,3 @@
 InputSectionBase<ELFT> *
-InputSectionBase<ELFT>::getRelocTarget(const Elf_Rel &Rel) {
+InputSectionBase<ELFT>::getRelocTarget(const Elf_Rel &Rel) const {
   // Global symbol
----------------
rafael wrote:
> Can you commit the const fixes first?
Done.

================
Comment at: ELF/OutputSections.cpp:1151
@@ -1150,3 +1151,3 @@
       InputSectionBase<ELFT> *Target = S->getRelocTarget(*RelI);
-      if (Target != &InputSection<ELFT>::Discarded && Target->isLive()) {
         uint32_t CieOffset = Offset + 4 - ID;
----------------
rafael wrote:
> Can you commit the s/isLife/Life/ change first?
Done.

================
Comment at: ELF/Symbols.h:236
@@ +235,3 @@
+  // If this is null, the symbol is an absolute symbol.
+  InputSectionBase<ELFT> *&Section;
+
----------------
rafael wrote:
> Not sure I follow. Why is this not redundant with the Repl pointer in the section?
In many places we write Sym->Section to get an input section for the symbol. We either update all such occurrences with Sym->Section->Repl, or make Section a reference like this.


http://reviews.llvm.org/D17529