[PATCH] D99191: [WebAssembly][MC] Record limit constraints for table sizes

[Sam Clegg via Phabricator via llvm-commits]

sbc100 accepted this revision.
sbc100 added a comment.
This revision is now accepted and ready to land.

Great!



================
Comment at: llvm/include/llvm/MC/MCSymbolWasm.h:111
     return isTable() && hasTableType() &&
-           getTableType() == wasm::ValType::FUNCREF;
+           getTableType().ElemType == uint8_t(wasm::ValType::FUNCREF);
   }
----------------
Can you use `WASM_TYPE_FUNCREF` and avoid that cast?


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:860
         return true;
-      auto Type = parseType(TypeName);
-      if (!Type)
-        return error("Unknown type in .tabletype directive: ", TypeTok);
+      auto ElemType = parseType(ElemTypeName);
+      if (!ElemType)
----------------
Using auto all over the place makes this harder to review and goes against llvm policy.   I guess its pre-existing code so you don't need to change it here.. but I wish it this had a type name


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:868
+          return true;
+      }
 
----------------
make this into single condition (with no curly braces)?


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:874
       WasmSym->setType(wasm::WASM_SYMBOL_TYPE_TABLE);
-      WasmSym->setTableType(Type.getValue());
+      wasm::WasmTableType Type = {uint8_t(ElemType.getValue()), Limits};
+      WasmSym->setTableType(Type);
----------------
All these casts make me this that WasmTableType should have ValType rather than uint8_t


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99191