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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

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 at: llvm/lib/Target/WebAssembly/Disassembler/WebAssemblyDisassembler.cpp:201
     case WebAssembly::OPERAND_FUNCTION32:
+    case WebAssembly::OPERAND_TABLE32:
     case WebAssembly::OPERAND_OFFSET32:
----------------
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.


================
Comment at: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyMCTargetDesc.h:79
   OPERAND_BRLIST,
+  /// 32-bit unsigned table indices.
+  OPERAND_TABLE32,
----------------
I think "table index" in ambiguous.   Maybe "table number"?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:14
+
+// Instructions that handle tables
+
----------------
These two comments above can probably just be a single comment, no?  (unless I'm missing something?)


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:65
+
+
----------------
Remove trailing newlines.


================
Comment at: llvm/test/MC/WebAssembly/tables.s:80
+# CHECK-NEXT:.Ltmp0:
+# CHECK-NEXR:	.size	copy_tables, .Ltmp0-copy_tables
+
----------------
I think its common to put the expectations for each function inside or alongside the function itself.  

But I admit we are not very consistent in this directory.

What do you think about the style uses in `MC/WebAssembly/bulk-memory-encodings.s`or `simd-encodings.s`.

@tlively probably knows more about the right and wrong way to do this stuff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89797