This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] De-dup indirect function table
ClosedPublic

Authored by sbc100 on Dec 7 2017, 3:54 PM.

Details

Summary

Create the indirect function table based on symbols rather
than just duplicating the input entries. This has the
effect of de-duplicating the table.

This is a followup to the equivalent change made for globals:

https://reviews.llvm.org/D40859

Parially based on a patch by Nicholas Wilson:

https://reviews.llvm.org/D40845

Diff Detail

Repository
rL LLVM

Event Timeline

sbc100 created this revision.Dec 7 2017, 3:54 PM
sbc100 updated this revision to Diff 126067.Dec 7 2017, 4:00 PM
  • remove comment
ruiu added inline comments.Dec 7 2017, 4:17 PM
wasm/InputFiles.cpp
85 ↗(On Diff #126067)

nit: we don't use const in such a short function as it is obvious that it is not re-assigned.

193 ↗(On Diff #126067)

Can you add a comment to describe what this code block is expected to do?

wasm/InputFiles.h
115 ↗(On Diff #126067)

Forgot to mention in previous review threads, but we have ArrayRef class, and we usually use that class instead of const std::vector<T> &. You don't need to update it in this patch, but do you mind if I ask you update it in a follow-up patch?

wasm/Writer.cpp
238 ↗(On Diff #126067)

always

240 ↗(On Diff #126067)

useful

339 ↗(On Diff #126067)

nit: pre-increment is more conventional.

sbc100 updated this revision to Diff 126077.Dec 7 2017, 5:01 PM
  • feedback
ncw accepted this revision.Dec 8 2017, 3:10 AM

Fantastic! You've saved me some work, thanks. It's on my todo list for today to start splitting up the WIP Comdat reviews into smaller chunks. Plus, you've tidied it up along the way.

Could there be a test for a weakly-exported symbol, defined in two translation units, and which has its address taken in both? Something like this:

=== file 1
define weak void @weakFn() {
entry:
  ret void
}

define i32 @exportFn1() {
entry:
  ret i32 ptrtoint (void ()* @weakFn to i32)
}

=== file2

define weak void @weakFn() {
entry:
  ret void
}

define i32 @exportFn2() {
entry:
  ret i32 ptrtoint (void ()* @weakFn to i32)
}

=== Expected
  exportFn1() == exportFn2()
wasm/InputFiles.cpp
204 ↗(On Diff #126077)

In my patch, I'd added support for multiple element segments. It was probably a bit overkill, but the more Wasm syntax we can support, the smaller the linking conventions can be. Doesn't really matter, this is fine.

You're checking the segment offset is zero, but for completeness do you need to check Segment.Offset.Opcode == WASM_OPCODE_I32_CONST before reading from Segment.Offset.Value.Int32?

(Finally, a couple of typos: "with all symbols that are called indirectly", "contains more than one element segment".)

This revision is now accepted and ready to land.Dec 8 2017, 3:10 AM
sbc100 marked an inline comment as done.Dec 8 2017, 10:28 AM

Good call regarding that test, I added it here in a separate change so we can see how this change effects it:
https://reviews.llvm.org/D41024

wasm/InputFiles.cpp
204 ↗(On Diff #126077)

I'd rather limit support for multiple segments or non-zero-based segments because:
a) it simpler
b) clang has no use for it yet so always writes a single segment
c) its hard to produce test inputs because of (b).
d) we can always remove restrictions later

sbc100 updated this revision to Diff 126184.Dec 8 2017, 10:49 AM
sbc100 marked an inline comment as done.
  • rebase
sbc100 updated this revision to Diff 126221.Dec 8 2017, 3:15 PM
  • remove unused member
This revision was automatically updated to reflect the committed changes.
ruiu added inline comments.Dec 12 2017, 5:30 PM
lld/trunk/wasm/InputFiles.cpp
142

This function is getting longer. Please consider splitting it into multiple functions in a follow-up patch.

200–209

Could you add blank lines more often to split code into meaningful code blocks? For example, I'd add two blank lines at line 201 and line 207 so that the code is organized as (1) initialization, (2) error checking and (3) TableSymbols initialization.