Adds more testing in basic-assembly.s and a new test tables.s.
Adds support to yaml reading and writing of tables as well.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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.
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 |
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. |
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
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.
For other module elements that include Index it looks like it is the first elem in the struct (see Event and Function below)