[PATCH] D88815: Added .tabletype to asm and multiple table support in obj files

[Sam Clegg via Phabricator via llvm-commits]

sbc100 accepted this revision.
sbc100 added a comment.

Looks great!  Just a couple of nits



================
Comment at: llvm/include/llvm/MC/MCSymbolWasm.h:114
 
+  const uint8_t &getTableType() const {
+    assert(TableType.hasValue());
----------------
Should we use `ValType` for table type?  


================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:1529
+        // FIX (pmatos): Which limits should by default be imposed to tables?
+        Table.Limits = {wasm::WASM_LIMITS_FLAG_NONE, 0, 0};
+
----------------
Presumably the limits should be part of the type eventually?


================
Comment at: llvm/lib/ObjectYAML/WasmEmitter.cpp:365
       writeUint32(OS, Import.EventImport.SigIndex);
-      NumImportedGlobals++;
+      NumImportedEvents++;
       break;
----------------
Oops..  I guess we don't have enough testing here.


================
Comment at: llvm/lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp:84
+     << WebAssembly::typeToString(
+            static_cast<wasm::ValType>(Sym->getTableType()));
+  OS << '\n';
----------------
Would save this static cast too if that was ValType maybe?


================
Comment at: llvm/test/MC/WebAssembly/tables.s:43
+# BIN-NEXT:        Table:           2
\ No newline at end of file

----------------
Add newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88815