This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Optionally compress relocation sites in the code section
ClosedPublic

Authored by sbc100 on May 3 2018, 6:49 PM.

Details

Summary

This change adds the ability for lld to remove LEB padding from
code section. This effectively shrinks the size of the resulting binary
in proportion to the number of code relocations.

Since there will be a performance cost this is currently only active
for -O2. Some toolchains may instead want to perform this compression
as a post linker step (for example running a binary through binaryen will
automatically compress these values).

I imagine we might want to make this the default in the future and
force people to opt out with -O0.

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.May 3 2018, 6:49 PM
sbc100 updated this revision to Diff 145133.May 3 2018, 6:53 PM
  • cleanup
sbc100 updated this revision to Diff 145134.May 3 2018, 6:53 PM
  • cleanup
Harbormaster completed remote builds in B17678: Diff 145134.
sbc100 updated this revision to Diff 145135.May 3 2018, 6:55 PM
  • cleanup

I will need to update this patch to handle FUNCTION_OFFSET relocations once we land https://reviews.llvm.org/D45118

sbc100 updated this revision to Diff 145305.May 4 2018, 3:09 PM

Add comment

sbc100 edited the summary of this revision. (Show Details)May 4 2018, 3:15 PM
sbc100 added a reviewer: ncw.
sbc100 retitled this revision from [WebAssembly] Add option to remove LEB padding at relocate sites to [WebAssembly] Add option to remove LEB padding at relocation sites.
sbc100 updated this revision to Diff 145307.May 4 2018, 3:17 PM
  • remove some asserts
sbc100 updated this revision to Diff 145616.May 7 2018, 6:48 PM
  • no need to new flag
sbc100 edited the summary of this revision. (Show Details)May 7 2018, 6:50 PM
sbc100 updated this revision to Diff 145618.May 7 2018, 6:53 PM
  • add comment
sbc100 updated this revision to Diff 145620.May 7 2018, 6:54 PM
  • revert
ruiu added a subscriber: ruiu.May 7 2018, 6:55 PM

Overall looking good, but I think I have a concern about the lack of comments. Could you expand comments to make the .cpp file more self-explanatory so that a first-time reader who's not very familiar with wasm can understand the code by reading it?

sbc100 edited reviewers, added: ruiu; removed: jgravelle-google.May 7 2018, 6:57 PM
sbc100 retitled this revision from [WebAssembly] Add option to remove LEB padding at relocation sites to [WebAssembly] Optionally compress relocation sites in the code section.
ncw accepted this revision.May 8 2018, 6:54 AM

This looks like a nice size optimisation, great.

This revision is now accepted and ready to land.May 8 2018, 6:54 AM
sbc100 updated this revision to Diff 145734.May 8 2018, 11:06 AM
  • add comments

OK to land now @ruiu ?

Planning on landing this today unless there are any more comments.

ruiu added a comment.May 18 2018, 3:19 PM

I'm sorry, I missed your ping.

wasm/Driver.cpp
292 ↗(On Diff #145734)

I think 0 should be default. For example, if you don't pass -O to gcc or clang, they emit a non-optimized code.

wasm/InputChunks.cpp
184 ↗(On Diff #145734)

When a buffer is given, it is usually the first argument. It is better to follow the convention for readability.

232 ↗(On Diff #145734)

Can you also mention that this function only computes the length of the compressed section and doesn't actually write anything?

sbc100 updated this revision to Diff 147612.May 18 2018, 4:17 PM
sbc100 marked 3 inline comments as done.
  • feedback
ruiu accepted this revision.May 18 2018, 4:25 PM

LGTM

This revision was automatically updated to reflect the committed changes.