Page MenuHomePhabricator

[WebAssembly] Only emit indirect function table import if needed
ClosedPublic

Authored by wingo on Tue, Nov 17, 8:52 AM.

Details

Summary

The indirect function table, synthesized by the linker, is needed if and
only if there are TABLE_INDEX relocs.

Diff Detail

Event Timeline

wingo created this revision.Tue, Nov 17, 8:52 AM
wingo requested review of this revision.Tue, Nov 17, 8:52 AM

I have run CodeGen/WebAssembly, MC/WebAssembly, and lld/test/wasm tests, which pass. Split off from https://reviews.llvm.org/D90948.

wingo added a comment.Tue, Nov 17, 8:55 AM

Still has the bad aspect of doing function table symbol creation in MCContext. Will look deeper there tomorrow. Does this size of the change look OK to you, @sbc100 ?

This looks pretty much like I was suggesting, although I was hoping we could make it more isolated by just making the creation of the existing table import conditional rather than creating a new MC symbol.

llvm/lib/MC/WasmObjectWriter.cpp
506

Can we just move the code below, or make it conditional to avoid the MCConteext changes?

(i.e. can we make this change local to this file?)

What I was hoping for here is minimal change to the object writer to remove the table from object files that don't reference it. This seem separable from the question of whether the indirection function table should be first class MC symbol .. although maybe I'm taking this CL splitting thing to far.

llvm/test/MC/WebAssembly/weak-alias.s
242 ↗(On Diff #305814)

Why did these changes occur? Do you have some tool to automatically updating the expected output?

wingo added a comment.EditedThu, Nov 19, 12:08 AM

What I was hoping for here is minimal change to the object writer to remove the table from object files that don't reference it. This seem separable from the question of whether the indirection function table should be first class MC symbol .. although maybe I'm taking this CL splitting thing to far.

Just backing up a bit for context -- the end goal to add support to wasm-ld for linking object files that reference tables; pruning needless __indirect_function_table imports is a "side quest". To link tables, I need relocs for all table references written to object files, and linker symtab entries for all tables. Do you agree that a first-class MCSymbol is a part of that final pipeline? If so, I think we're on the good path here, and that using a different mechanism would just require more testing when we put the MCSymbol mechanism in place. Anyway, just a gentle argument that it seems to me that this is about the right patch size, though whether we have the symbol creation in MCContext is another question of course.

llvm/lib/MC/WasmObjectWriter.cpp
506

We certainly can. However, with the next patch, we will need to call getOrCreateWasmDefaultFunctionTableSymbol from three places in Target/WebAssembly: the asm parser and from the two instruction selectors. Do you prefer to duplicate this code there?

llvm/test/MC/WebAssembly/weak-alias.s
242 ↗(On Diff #305814)

I just checked and these changes are unnecessary. I will remove this change from this patch.

What is going on here fwiw is that the weak-alias test will change when table numbers get encoded as 5-byte relocs, so this file was on the changeset for the TABLE_NUMBER patch. I updated that test by pasting in the output of the obj2yaml with the prefix. That's where these extraneous bits come in.

When I removed the test changes corresponding to the TABLE_NUMBER patch, we're just left with this meaningless update.

wingo updated this revision to Diff 306324.Thu, Nov 19, 12:09 AM
  • Remove needless change to weak-alias.s

What I was hoping for here is minimal change to the object writer to remove the table from object files that don't reference it. This seem separable from the question of whether the indirection function table should be first class MC symbol .. although maybe I'm taking this CL splitting thing to far.

Just backing up a bit for context -- the end goal to add support to wasm-ld for linking object files that reference tables; pruning needless __indirect_function_table imports is a "side quest". To link tables, I need relocs for all table references written to object files, and linker symtab entries for all tables. Do you agree that a first-class MCSymbol is a part of that final pipeline? If so, I think we're on the good path here, and that using a different mechanism would just require more testing when we put the MCSymbol mechanism in place. Anyway, just a gentle argument that it seems to me that this is about the right patch size, though whether we have the symbol creation in MCContext is another question of course.

Yes, I was thinking we could just land the side quest (I like that term!) quickly without too much discussion. If you want to include the MCSymbol creation in addtion then I'm not averse to that.. but as you say, that means we have to figure if the target-independent MCContext is the right place for the two new function (getOrCreateWasmFunctionTableSymbol). My gut tells me it not, but I'm also no an expert in these areas... @tlively @aardappel might have some idea where the right place for this kind of thing is.

wingo updated this revision to Diff 306326.Thu, Nov 19, 12:32 AM
  • Move __indirect_function_table reference to WasmObjectWriter
wingo added inline comments.Thu, Nov 19, 12:35 AM
llvm/lib/MC/WasmObjectWriter.cpp
506

In the end I think you are right and that in this patch, no need to muck with MCContext.h. If we should factor out a function, it will become obvious in future patches.

sbc100 accepted this revision.Sat, Nov 21, 11:31 PM
sbc100 added inline comments.
llvm/test/MC/WebAssembly/reloc-pic.s
97

What happened here?

llvm/test/MC/WebAssembly/tables.s
211

Where these here before but just not being checked for I guess?

This revision is now accepted and ready to land.Sat, Nov 21, 11:31 PM
sbc100 added inline comments.Sat, Nov 21, 11:33 PM
llvm/lib/MC/WasmObjectWriter.cpp
503

Perhaps add a TODO here since we probably don't to keep this here long right?

wingo added inline comments.Tue, Nov 24, 7:31 AM
llvm/lib/MC/WasmObjectWriter.cpp
503

We keep this one, I think. Currently call_indirect has no relocs. In the future it will have TABLE_NUMBER relocs. TABLE_INDEX relocs will stay around, used for function pointers.

In some future it might be useful to have the TABLE_INDEX relocs specify an "address space" in the form of a table number (table symbol reference); i.e. the EXTENDED_TABLE_INDEX reloc would be against both a function symbol and a table symbol. But until then, and until such a use case arises, I think TABLE_INDEX relocs still implicitly use __indirect_function_table. WDYT?

llvm/test/MC/WebAssembly/reloc-pic.s
97

This is the result of replacing the whole CHECK+CHECK_NEXT* block with the output of the command in question. Harmless AFAIU, but extraneous also, so I have removed it.

llvm/test/MC/WebAssembly/tables.s
211

I just checked and yes, they were here before and not being checked for.

wingo updated this revision to Diff 307354.EditedTue, Nov 24, 7:31 AM

Remove extraneous test change

wingo added a comment.Tue, Nov 24, 7:33 AM

If this looks good to you, feel very free to land :)

This patch doesn't seem to apply to the master branch of llvm.

It also says this when I run arc patch:

This diff is against commit e0257d84325828a4faddfaf48512358aa9681632, but
 the commit is nowhere in the working copy. Try to apply it against the
 current working copy state? (1a036e9cc82a7f6d6f4675d631fa5eecd8748784)
 [Y/n]

Perhaps you need to rebase?

This patch doesn't seem to apply to the master branch of llvm.

Conflicts with https://reviews.llvm.org/D90930 apparently; will fix.

It also says this when I run arc patch:

This diff is against commit e0257d84325828a4faddfaf48512358aa9681632, but
 the commit is nowhere in the working copy. Try to apply it against the
 current working copy state? (1a036e9cc82a7f6d6f4675d631fa5eecd8748784)
 [Y/n]

Perhaps you need to rebase?

Yeah the patch was on a local copy of the already-landed https://reviews.llvm.org/D91635. Will rebase,

sbc100 added inline comments.Wed, Nov 25, 8:04 AM
llvm/lib/Object/WasmObjectFile.cpp
505

We should probably get smarter about these over allocations now that we have so many different import types here.

Unrelated to this CL though.

Looks like we need one more rebase now that https://reviews.llvm.org/D91849 has landed.

wingo added a comment.Wed, Nov 25, 8:29 AM

Looks like we need one more rebase now that https://reviews.llvm.org/D91849 has landed.

done, thanks for your patience; MC, CodeGen, and lld WebAssembly tests checked and passing.

This revision was landed with ongoing or failed builds.Wed, Nov 25, 8:39 AM
This revision was automatically updated to reflect the committed changes.