[lld] r247963 - COFF: Optimize ICF by not creating temporary vectors.

[Sean Silva via llvm-commits]

On Thu, Sep 17, 2015 at 6:51 PM, Rui Ueyama via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: ruiu
> Date: Thu Sep 17 20:51:37 2015
> New Revision: 247963
>
> URL: http://llvm.org/viewvc/llvm-project?rev=247963&view=rev
> Log:
> COFF: Optimize ICF by not creating temporary vectors.
>
> Previously, ICF created a vector for each SectionChunk. The vector
> contained pointers to successors, which are namely associative sections
> and COMDAT relocation targets. The reason I created vectors is because
> I thought that that would make section comparison faster.
>
> It did make the comparison faster. When self-linking, for example, it
> saved about 10 ms on each iteration. The time we spent on constructing
> the vectors was 124 ms. If we iterate more than 12 times, return from
> the investment exceeds the initial cost.
>
> In reality, it usually needs 5 iterations. So we shouldn't construct
> the vectors.
>
> Modified:
>     lld/trunk/COFF/Chunks.h
>     lld/trunk/COFF/ICF.cpp
>
> Modified: lld/trunk/COFF/Chunks.h
> URL:
> http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/Chunks.h?rev=247963&r1=247962&r2=247963&view=diff
>
> ==============================================================================
> --- lld/trunk/COFF/Chunks.h (original)
> +++ lld/trunk/COFF/Chunks.h Thu Sep 17 20:51:37 2015
> @@ -203,7 +203,6 @@ private:
>
>    // Used for ICF (Identical COMDAT Folding)
>    void replaceWith(SectionChunk *Other);
> -  std::vector<SectionChunk *> Successors;
>    uint64_t GroupID;
>
>    // Chunks are basically unnamed chunks of bytes.
>
> Modified: lld/trunk/COFF/ICF.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/ICF.cpp?rev=247963&r1=247962&r2=247963&view=diff
>
> ==============================================================================
> --- lld/trunk/COFF/ICF.cpp (original)
> +++ lld/trunk/COFF/ICF.cpp Thu Sep 17 20:51:37 2015
> @@ -128,12 +128,22 @@ bool ICF::equalsConstant(const SectionCh
>  }
>
>  bool ICF::equalsVariable(const SectionChunk *A, const SectionChunk *B) {
> -  assert(A->Successors.size() == B->Successors.size());
> -  return std::equal(A->Successors.begin(), A->Successors.end(),
> -                    B->Successors.begin(),
> -                    [](const SectionChunk *X, const SectionChunk *Y) {
> -                      return X->GroupID == Y->GroupID;
> -                    });
> +  // Compare associative sections.
> +  for (size_t I = 0, E = A->AssocChildren.size(); I != E; ++I)
> +    if (A->AssocChildren[I]->GroupID != B->AssocChildren[I]->GroupID)
> +      return false;
> +
> +  // Compare relocations.
> +  auto Eq = [&](const coff_relocation &R1, const coff_relocation &R2) {
> +    SymbolBody *B1 = A->File->getSymbolBody(R1.SymbolTableIndex)->repl();
> +    SymbolBody *B2 = B->File->getSymbolBody(R2.SymbolTableIndex)->repl();
> +    if (B1 == B2)
> +      return true;
> +    auto *D1 = dyn_cast<DefinedRegular>(B1);
> +    auto *D2 = dyn_cast<DefinedRegular>(B2);
> +    return D1 && D2 && D1->getChunk()->GroupID == D2->getChunk()->GroupID;
> +  };
> +  return std::equal(A->Relocs.begin(), A->Relocs.end(),
> B->Relocs.begin(), Eq);
>  }
>

This looks very similar to `// Compare relocations.` in equalsConstant. I
see that they are different in a couple small ways though. Could they at
least be refactored to have a the same structure? equalsConstant uses:

    if (auto *D1 = dyn_cast<DefinedRegular>(B1))
      if (auto *D2 = dyn_cast<DefinedRegular>(B2))

equalsVariable uses:

D1 && D2 && ...

It would make it easier to see how they are different, by emphasizing how
they are the same.

Not a big deal, but it did take me some staring to identify the differences.

-- Sean Silva


>  bool ICF::partition(ChunkIterator Begin, ChunkIterator End, Comparator
> Eq) {
> @@ -191,16 +201,6 @@ void ICF::run() {
>                return A->GroupID < B->GroupID;
>              });
>
> -  // Initialize chunk successors for equalsVariable.
> -  for (SectionChunk *SC : SChunks) {
> -    SC->Successors = SC->AssocChildren;
> -    for (const coff_relocation &R : SC->Relocs) {
> -      SymbolBody *B = SC->File->getSymbolBody(R.SymbolTableIndex)->repl();
> -      if (auto D = dyn_cast<DefinedRegular>(B))
> -        SC->Successors.push_back(D->getChunk());
> -    }
> -  }
> -
>    // Split groups until we get a convergence.
>    int Cnt = 1;
>    forEachGroup(SChunks, equalsConstant);
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150917/7b9c8cf0/attachment.html>