This is an archive of the discontinued LLVM Phabricator instance.

COFF, ELF: Adjust ICF hash computation to account for self relocations.
ClosedPublic

Authored by pcc on Jan 18 2019, 8:38 PM.

Details

Summary

It turns out that sections in PGO instrumented object files on Windows
contain a large number of relocations pointing to themselves. With
r347429 this can cause many sections to receive the same hash (usually
zero) as a result of a section's hash being xor'ed with itself.

This patch causes the COFF and ELF linkers to avoid this problem
by adding the hash of the relocated section instead of xor'ing it.
On my machine this causes the regressing test case
provided by Mozilla to terminate in 2m41s.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Jan 18 2019, 8:38 PM
grimar accepted this revision.Jan 20 2019, 1:16 AM

I think this is OK. Please wait for Rui's opinion.

This revision is now accepted and ready to land.Jan 20 2019, 1:16 AM

When this lands, is it worth cherry-picking to 8.0?

pcc added a comment.Jan 22 2019, 1:17 PM

When this lands, is it worth cherry-picking to 8.0?

Yes, that's what my plan was.

ruiu added a comment.Jan 22 2019, 2:54 PM

Or maybe we can simply add hashes instead of xor'ing?

pcc added a comment.Jan 22 2019, 3:08 PM

Or maybe we can simply add hashes instead of xor'ing?

Yeah, that could work too. Although my first thought was that combining hashes that way could end up losing entropy, it doesn't seem like we'd be losing too much in practice because if you have (say) 31 self relocations you would only end up losing 5 bits of entropy,. I'll do an experiment to see how well that works.

pcc updated this revision to Diff 182988.Jan 22 2019, 3:38 PM
  • Add instead of xoring
pcc added a comment.Jan 22 2019, 3:41 PM

Done. Benchmark numbers appear the same for Chrome and Firefox (including with D56986, which would mean losing twice as much entropy (I think)).

ruiu accepted this revision.Jan 22 2019, 3:41 PM

LGTM

pcc edited the summary of this revision. (Show Details)Jan 22 2019, 3:42 PM
This revision was automatically updated to reflect the committed changes.
pcc added a comment.Jan 22 2019, 4:02 PM

Merge requested in PR40416.