Implementation of instructions table.get, table.set, table.grow,
table.size, table.fill, table.copy.
Missing instructions are table.init and elem.drop as they deal with
element sections which are not yet implemented.
Added more tests to tables.s
Paths
| Differential D89797
[WebAssembly] Implementation of (most) table instructions ClosedPublic Authored by pmatos on Oct 20 2020, 8:25 AM.
Details Summary Implementation of instructions table.get, table.set, table.grow, Missing instructions are table.init and elem.drop as they deal with Added more tests to tables.s
Diff Detail
Event TimelineComment Actions So the goal here is to add most of the table instructions. This introduces some table relocations, which (in a following patch) we will resolve in the linker. I have some concerns with this patch. The tests here are testing that instructions table.* written in .s files will be assembled properly into an object file. However, there are no tests actually testing the register based part of the instruction definitions. So, for example for table.grow, I am unsure that (outs i32imm_op:$sz), (ins table32_op:$table, i32imm_op:$n, reftype_op:$val) actually makes sense for the register based version of the instruction and I have no tests actually looking into it since I have been mostly concerned at assembler level. Should we add intrinsics for these? and try to test this at codegen level? or just add a comment pointing out that register-level instruction definitions are currently untested? or something else? Suggestions welcome. Comment Actions
Yes, I would typically add intrinsics for something like this and test codegen from them. This would require being able to represent reference types in LLVM IR, though, so I would be ok with leaving that to the future. For now if you pass -show-encoding to llvm-mc you can at least test that the binary encoding is what you expect it to be. See simd-encodings.s for an example of that.
Comment Actions In general I'm loving how small this change is. I don't really grok the tablegen stuff but everything else looks pretty good to me.
Comment Actions Thanks for the review - I will submit a new revision.
Comment Actions These changes reflect the first set of comments. High on the list is quite a few changes related to setting registers for funcrefs and externrefs. A few things might not be strictly necessary but pave the way for future work. I was tempted to implement the intrinsics, but I think I will leave for a next patch. Sam mentions one of the things he likes about the patch is that it's small, so lets not mess with that. :) Comment Actions Regarding linter clang-format suggestions - should I reformat the code as suggested or leave it? Comment Actions No I wouldn't format any code you didn't write.. nor assume that clang-format always knows best in all cases. I normally run git clang-format origin/master to ensure that the lines I've touched are formatted correctly.
Comment Actions Is this good to merge?
Comment Actions This mostly lgtm. I would love it if we could avoid adding the new MVT value for now since then we could avoid changing the shared code on llvm/lib/CodeGen/ValueTypes.cpp.. but perhaps that absolutely needed for this change? lgtm if @tlively is OK with the .td changes.
Comment Actions Looks good to me with @sbc100's final comment addressed 🎉
This revision is now accepted and ready to land.Oct 22 2020, 9:36 PM
This revision was landed with ongoing or failed builds.Oct 23 2020, 8:43 AM Closed by commit rG69e2797eaed5: [WebAssembly] Implementation of (most) table instructions (authored by pmatos, committed by sbc100). · Explain Why This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 300313 llvm/include/llvm/BinaryFormat/WasmRelocs.def
llvm/include/llvm/CodeGen/ValueTypes.td
llvm/include/llvm/Object/Wasm.h
llvm/include/llvm/Support/MachineValueType.h
llvm/lib/CodeGen/ValueTypes.cpp
llvm/lib/MC/WasmObjectWriter.cpp
llvm/lib/Object/RelocationResolver.cpp
llvm/lib/Object/WasmObjectFile.cpp
llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCCodeEmitter.cpp
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h
llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyWasmObjectWriter.cpp
llvm/lib/Target/WebAssembly/WebAssemblyExplicitLocals.cpp
llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp
llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td
llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td
llvm/lib/Target/WebAssembly/WebAssemblyRegisterInfo.td
llvm/test/MC/WebAssembly/tables.s
llvm/utils/TableGen/CodeGenTarget.cpp
|
This doesn't need the 32 suffix.. i think its more like OPERAND_GLOBAL. I can't imagine us ever needing 2^^64 different tables... touch wood.