[PATCH] D65242: [ELF] More dynamic relocation packing

[Rui Ueyama via llvm-commits]

Looks like Phabricator is having a problem, so I'll review this patch in
emai.

On Thu, Jul 25, 2019 at 6:10 AM Vic Yang via Phabricator <
reviews at reviews.llvm.org> wrote:

> victoryang created this revision.
> victoryang added a reviewer: ruiu.
> Herald added subscribers: llvm-commits, mgrang, MaskRay, kristof.beyls,
> arichardson, javed.absar, emaste.
> Herald added a reviewer: espindola.
> Herald added a project: LLVM.
>
> Currently, with Android dynamic relocation packing, only relative
> relocations are grouped together.  This patch implements similar
> packing for non-relative relocations.
>
> The implementation groups non-relative relocations with the same
> r_info.  By requiring a minimum group size of 3, this achieves
> smaller relocation sections.  Linking libmonochrome in Chromium for
> Android, targeting ARM32, I see a .rel.dyn of size 127697 bytes, as
> compared to 150532 bytes without this change.
>
> Grouping by r_info also allows the runtime dynamic linker to implement
> an 1-entry cache to reduce the number of symbol lookup required.  Such
> a cache would hit 11167 times for libmonochrome, as compared to 4107
> times without this change.
>
>
> Repository:
>   rLLD LLVM Linker
>
> https://reviews.llvm.org/D65242
>
> Files:
>   lld/ELF/SyntheticSections.cpp
>
>
> Index: lld/ELF/SyntheticSections.cpp
> ===================================================================
> --- lld/ELF/SyntheticSections.cpp
> +++ lld/ELF/SyntheticSections.cpp
> @@ -1679,6 +1679,35 @@
>        relativeGroups.emplace_back(std::move(group));
>    }
>
> +  // Sort by r_info first so that we can group relocations with the same
> r_info.
> +  // The cost of each group varies based on r_info, but we can
> approximate the
> +  // cost analysis by the number of values encoded.  Each group adds 3
> values to
> +  // be encoded, and each relocation in a group encodes one less value,
> so we
> +  // only group relocations if there are 3 or more of them with the same
> r_info.
> +  llvm::sort(nonRelatives, [](const Elf_Rela &a, const Elf_Rela &b) {
> +    return a.r_info < b.r_info;
> +  });
>

A r_info consists of a symbol offset and a relocation type. If I understand
correctly, you basically want to sort symbols by symbol offset -- am I
correct? If so, maybe you want to make it explicit by sorting only by
symbol offset?

I also found this comment little bit confusing -- the 2nd line and after
that it explains the cost of something, but what is the cost? Or, first of
all, why do you want to sort relocations by r_info? I'd explain that first.

+  std::vector<Elf_Rela> ungroupedNonRelatives;
> +  std::vector<std::vector<Elf_Rela>> nonRelativeGroups;
> +  for (auto i = nonRelatives.begin(), e = nonRelatives.end(); i != e;) {
> +    std::vector<Elf_Rela> group;
> +    do {
> +      group.push_back(*i++);
> +    } while (i != e && (i - 1)->r_info == i->r_info);
> +
> +    if (group.size() < 3)
> +      ungroupedNonRelatives.insert(ungroupedNonRelatives.end(),
> group.begin(),
> +                                   group.end());
> +    else
> +      nonRelativeGroups.emplace_back(std::move(group));
> +  }
> +
> +  // Sort ungrouped relocations by offset to minimize the encoded length.
> +  llvm::sort(ungroupedNonRelatives, [](const Elf_Rela &a, const Elf_Rela
> &b) {
> +    return a.r_offset < b.r_offset;
> +  });
> +
>    unsigned hasAddendIfRela =
>        config->isRela ? RELOCATION_GROUP_HAS_ADDEND_FLAG : 0;
>
> @@ -1733,14 +1762,26 @@
>      }
>    }
>
> -  // Finally the non-relative relocations.
> -  llvm::sort(nonRelatives, [](const Elf_Rela &a, const Elf_Rela &b) {
> -    return a.r_offset < b.r_offset;
> -  });
> -  if (!nonRelatives.empty()) {
> -    add(nonRelatives.size());
> +  // Grouped non-relatives.
> +  for (std::vector<Elf_Rela> &g : nonRelativeGroups) {
> +    add(g.size());
> +    add(RELOCATION_GROUPED_BY_INFO_FLAG | hasAddendIfRela);
> +    add(g[0].r_info);
> +    for (Elf_Rela &r : g) {
> +      add(r.r_offset - offset);
> +      offset = r.r_offset;
> +      if (config->isRela) {
> +        add(r.r_addend - addend);
> +        addend = r.r_addend;
> +      }
> +    }
> +  }
> +
> +  // Finally the ungrouped non-relative relocations.
> +  if (!ungroupedNonRelatives.empty()) {
> +    add(ungroupedNonRelatives.size());
>      add(hasAddendIfRela);
> -    for (Elf_Rela &r : nonRelatives) {
> +    for (Elf_Rela &r : ungroupedNonRelatives) {
>        add(r.r_offset - offset);
>        offset = r.r_offset;
>        add(r.r_info);
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190729/9874d3e4/attachment.html>