<div dir="ltr"><div dir="ltr">Looks like Phabricator is having a problem, so I'll review this patch in emai.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jul 25, 2019 at 6:10 AM Vic Yang via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">victoryang created this revision.<br>
victoryang added a reviewer: ruiu.<br>
Herald added subscribers: llvm-commits, mgrang, MaskRay, kristof.beyls, arichardson, javed.absar, emaste.<br>
Herald added a reviewer: espindola.<br>
Herald added a project: LLVM.<br>
<br>
Currently, with Android dynamic relocation packing, only relative<br>
relocations are grouped together. This patch implements similar<br>
packing for non-relative relocations.<br>
<br>
The implementation groups non-relative relocations with the same<br>
r_info. By requiring a minimum group size of 3, this achieves<br>
smaller relocation sections. Linking libmonochrome in Chromium for<br>
Android, targeting ARM32, I see a .rel.dyn of size 127697 bytes, as<br>
compared to 150532 bytes without this change.<br>
<br>
Grouping by r_info also allows the runtime dynamic linker to implement<br>
an 1-entry cache to reduce the number of symbol lookup required. Such<br>
a cache would hit 11167 times for libmonochrome, as compared to 4107<br>
times without this change.<br>
<br>
<br>
Repository:<br>
rLLD LLVM Linker<br>
<br>
<a href="https://reviews.llvm.org/D65242" rel="noreferrer" target="_blank">https://reviews.llvm.org/D65242</a><br>
<br>
Files:<br>
lld/ELF/SyntheticSections.cpp<br>
<br>
<br>
Index: lld/ELF/SyntheticSections.cpp<br>
===================================================================<br>
--- lld/ELF/SyntheticSections.cpp<br>
+++ lld/ELF/SyntheticSections.cpp<br>
@@ -1679,6 +1679,35 @@<br>
relativeGroups.emplace_back(std::move(group));<br>
}<br>
<br>
+ // Sort by r_info first so that we can group relocations with the same r_info.<br>
+ // The cost of each group varies based on r_info, but we can approximate the<br>
+ // cost analysis by the number of values encoded. Each group adds 3 values to<br>
+ // be encoded, and each relocation in a group encodes one less value, so we<br>
+ // only group relocations if there are 3 or more of them with the same r_info.<br>
+ llvm::sort(nonRelatives, [](const Elf_Rela &a, const Elf_Rela &b) {<br>
+ return a.r_info < b.r_info;<br>
+ });<br></blockquote><div><br></div><div>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?</div><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+ std::vector<Elf_Rela> ungroupedNonRelatives;<br>
+ std::vector<std::vector<Elf_Rela>> nonRelativeGroups;<br>
+ for (auto i = nonRelatives.begin(), e = nonRelatives.end(); i != e;) {<br>
+ std::vector<Elf_Rela> group;<br>
+ do {<br>
+ group.push_back(*i++);<br>
+ } while (i != e && (i - 1)->r_info == i->r_info);<br>
+<br>
+ if (group.size() < 3)<br>
+ ungroupedNonRelatives.insert(ungroupedNonRelatives.end(), group.begin(),<br>
+ group.end());<br>
+ else<br>
+ nonRelativeGroups.emplace_back(std::move(group));<br>
+ }<br>
+<br>
+ // Sort ungrouped relocations by offset to minimize the encoded length.<br>
+ llvm::sort(ungroupedNonRelatives, [](const Elf_Rela &a, const Elf_Rela &b) {<br>
+ return a.r_offset < b.r_offset;<br>
+ });<br>
+<br>
unsigned hasAddendIfRela =<br>
config->isRela ? RELOCATION_GROUP_HAS_ADDEND_FLAG : 0;<br>
<br>
@@ -1733,14 +1762,26 @@<br>
}<br>
}<br>
<br>
- // Finally the non-relative relocations.<br>
- llvm::sort(nonRelatives, [](const Elf_Rela &a, const Elf_Rela &b) {<br>
- return a.r_offset < b.r_offset;<br>
- });<br>
- if (!nonRelatives.empty()) {<br>
- add(nonRelatives.size());<br>
+ // Grouped non-relatives.<br>
+ for (std::vector<Elf_Rela> &g : nonRelativeGroups) {<br>
+ add(g.size());<br>
+ add(RELOCATION_GROUPED_BY_INFO_FLAG | hasAddendIfRela);<br>
+ add(g[0].r_info);<br>
+ for (Elf_Rela &r : g) {<br>
+ add(r.r_offset - offset);<br>
+ offset = r.r_offset;<br>
+ if (config->isRela) {<br>
+ add(r.r_addend - addend);<br>
+ addend = r.r_addend;<br>
+ }<br>
+ }<br>
+ }<br>
+<br>
+ // Finally the ungrouped non-relative relocations.<br>
+ if (!ungroupedNonRelatives.empty()) {<br>
+ add(ungroupedNonRelatives.size());<br>
add(hasAddendIfRela);<br>
- for (Elf_Rela &r : nonRelatives) {<br>
+ for (Elf_Rela &r : ungroupedNonRelatives) {<br>
add(r.r_offset - offset);<br>
offset = r.r_offset;<br>
add(r.r_info);<br>
<br>
<br>
</blockquote></div></div>