Page MenuHomePhabricator

[WebAssembly] Added .tabletype to asm and multiple table support in obj files
ClosedPublic

Authored by pmatos on Mon, Oct 5, 3:01 AM.

Details

Summary

Adds more testing in basic-assembly.s and a new test tables.s.
Adds support to yaml reading and writing of tables as well.

Diff Detail

Event Timeline

pmatos created this revision.Mon, Oct 5, 3:01 AM
pmatos requested review of this revision.Mon, Oct 5, 3:01 AM
tlively accepted this revision.Mon, Oct 5, 8:47 AM

Looks good to me! I would want to wait and see what @sbc100 thinks, too. It would also be good to add that missing newline in tables.s.

This revision is now accepted and ready to land.Mon, Oct 5, 8:47 AM
sbc100 accepted this revision.Wed, Oct 7, 1:30 PM

Looks great! Just a couple of nits

llvm/include/llvm/MC/MCSymbolWasm.h
114

Should we use ValType for table type?

llvm/lib/MC/WasmObjectWriter.cpp
1529

Presumably the limits should be part of the type eventually?

llvm/lib/ObjectYAML/WasmEmitter.cpp
365

Oops.. I guess we don't have enough testing here.

llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp
84

Would save this static cast too if that was ValType maybe?

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

Add newline

pmatos added a comment.Fri, Oct 9, 3:39 AM

Thanks for the reviews. I will fix the nits mentioned and update the revision.

pmatos marked 3 inline comments as done.Mon, Oct 12, 7:40 AM
pmatos added inline comments.
llvm/lib/MC/WasmObjectWriter.cpp
1529

I agree but I can work on moving the limits in a future patch, if that's fine with you.

pmatos updated this revision to Diff 297601.Mon, Oct 12, 8:24 AM

Fix review nits

sbc100 accepted this revision.Mon, Oct 12, 8:35 AM
sbc100 added inline comments.
llvm/include/llvm/BinaryFormat/Wasm.h
74

For other module elements that include Index it looks like it is the first elem in the struct (see Event and Function below)

llvm/lib/ObjectYAML/WasmYAML.cpp
306

Move this so its first?

pmatos updated this revision to Diff 297631.Mon, Oct 12, 10:35 AM

Move Index to first position in WasmTable struct

pmatos marked 2 inline comments as done.Mon, Oct 12, 10:37 AM
pmatos added inline comments.
llvm/include/llvm/BinaryFormat/Wasm.h
74

That makes total sense. Changed.

llvm/lib/ObjectYAML/WasmYAML.cpp
306

Done.

pmatos updated this revision to Diff 297643.Mon, Oct 12, 11:05 AM
pmatos marked 2 inline comments as done.

Fix build failure

sbc100 accepted this revision.Mon, Oct 12, 12:01 PM

Is this ready to land now?

pmatos updated this revision to Diff 297681.Mon, Oct 12, 1:28 PM

Fix tests to include table index in expected dump

Is this ready to land now?

There were some tests failing due to mismatched expected output.
In all of them I missed adding the "Index: 0" to the table output. If tests pass in harbormaster then we are good to land.

It looks like there are a bunch of wasm-ld tests that need similar updates.

$ ../llvm-build/bin/llvm-lit lld/test/wasm/
...
...
Failed Tests (18):
  lld :: wasm/alias.s
  lld :: wasm/call-indirect.ll
  lld :: wasm/export-table.test
  lld :: wasm/growable-table.test
  lld :: wasm/import-table.test
  lld :: wasm/local-symbols.ll
  lld :: wasm/locals-duplicate.test
  lld :: wasm/pie.ll
  lld :: wasm/relocatable.ll
  lld :: wasm/shared-memory-no-atomics.yaml
  lld :: wasm/shared-memory.yaml
  lld :: wasm/shared.ll
  lld :: wasm/stack-pointer.ll
  lld :: wasm/undefined-weak-call.ll
  lld :: wasm/weak-alias-overide.ll
  lld :: wasm/weak-alias.ll
  lld :: wasm/weak-symbols.ll
  lld :: wasm/weak-undefined.ll
pmatos updated this revision to Diff 297763.Mon, Oct 12, 9:48 PM

Fix a bunch more lld tests to include index for tables

I seeing:

llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:111:11: warning: enumeration value 'FUNCREF' not handled in switch [-Wswitch]
  switch (VT) {

Other than that this looks good to land

I seeing:

llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:111:11: warning: enumeration value 'FUNCREF' not handled in switch [-Wswitch]
  switch (VT) {

Oh, I was not getting that. It seems to come from https://github.com/llvm/llvm-project/commit/3bba91f64eef15956f589fa446c265a714cc7893 and I hadn't pulled that in yet. If I rebase my patch on this then yes, I can see the warning. i will fix it in this patch. No worries.

pmatos updated this revision to Diff 297785.Tue, Oct 13, 1:35 AM

Fix warning due to missing FUNCREF in switch

I seeing:

llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:111:11: warning: enumeration value 'FUNCREF' not handled in switch [-Wswitch]
  switch (VT) {

Oh, I was not getting that. It seems to come from https://github.com/llvm/llvm-project/commit/3bba91f64eef15956f589fa446c265a714cc7893 and I hadn't pulled that in yet. If I rebase my patch on this then yes, I can see the warning. i will fix it in this patch. No worries.

All looks good now.

pmatos updated this revision to Diff 297811.Tue, Oct 13, 3:16 AM

Further test fixes - missing index

pmatos retitled this revision from Added .tabletype to asm and multiple table support in obj files to [WebAssembly] Added .tabletype to asm and multiple table support in obj files.Tue, Oct 13, 7:45 AM
This revision was landed with ongoing or failed builds.Tue, Oct 13, 7:52 AM
This revision was automatically updated to reflect the committed changes.