[PATCH] D89797: [WebAssembly] Implementation of (most) table instructions

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

In D89797#2341916 <https://reviews.llvm.org/D89797#2341916>, @pmatos wrote:

> 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.

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 at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:169-170
+
+let OperandType = "OPERAND_REFTYPE" in
+def reftype_op : Operand<i32>;
+
----------------
Rather than a new Operand, this should probably be a new register class defined in WebAssemblyRegisterInfo.td. It will also probably make sense for now to have two new register types, one for externref and one for funcref rather than trying to combine them. Then there should be two versions of each table instruction, one for externref and one for tableref, even though those will lower to the same WebAssembly instruction. For an example of something similar, see LOCAL_GET_* in WebAssemblyInstrInfo.td.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:24
+
+defm TABLE_SET : I<(outs), (ins table32_op:$table, reftype_op:$val, i32imm_op:$i),
+                   (outs), (ins table32_op:$table),
----------------
$i should be an I32 (i.e. an i32 stack value), not an i32imm_op (i.e. a constant immediate argument). Same for i32s in the instructions below. Similarly, $val should be either EXTERNREF or FUNCREF once those register classes are defined.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89797/new/

https://reviews.llvm.org/D89797