[PATCH] D104945: [WebAssembly] Added initial type checker to MC Assembler

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:1361
+  assert(false && "mnemonic not found");
+  return StringRef();
+}
----------------
aardappel wrote:
> sbc100 wrote:
> > I surprised this functionality doesn't exist somewhere else already..
> Nope.. I definitely looked.
> 
> This stuff is really ugly though, it does a linear search thru the table, and then the type checker above does linear if-checks against the resulting string.
> 
> Would be awesome to not do this.
> 
> We could check directly against the opcode, but there's one opcode per type (8 or so of them), and then of course occasionally we make new types. So this would make for a very fast but very fragile switch-case above.
> 
> 3rd option is to generate some new table thru tablegen that has the exact mapping we need.
> 
> Opinions?
This method is really only used by DumpStack, which is only used under LLVM_DEBUG, right? linear search might not be so bad in that case.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:243
+  // Type Checker:
+  struct TypeStack {
+    SmallVector<wasm::ValType, 8> Stack;
----------------
why is Stack wrapped inside another struct (instead of a typedef or `using`)? I don't see any other uses of `TypeStack` anywhere other than just via `TS`. 


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:1012
 
+  void DumpTypeStack(Twine Msg) {
+    LLVM_DEBUG({
----------------
I think you can use `LLVM_DUMP_METHOD` rather than wrapping this whole method in `LLVM_DEBUG`.  I forget exactly what that does (it's not exactly the same as LLVM_DEBUG) but might still do what you want.

edit: based on how it's used below, I think you'd still need to wrap calls in `LLVM_DEBUG()` which might be what you're trying to avoid now. So I'll leave it up to you to decide whether you want to do that, but leave this comment here in case you weren't aware of `LLVM_DUMP_METHOD`


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:1156
+      if (PopType(ErrorLoc, wasm::ValType::I32)) return true;
+      if (CheckSig(ErrorLoc, LastSig)) return true;
+    } else if (Name == "call" || Name == "return_call") {
----------------
Is this right? function signatures for indirect calls aren't checked until runtime?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:357
+void WebAssemblyAsmPrinter::emitEndOfAsmFile(Module &M) {
+  emitExternalDecls(M);
 
----------------
I guess this only needs to run here in the case that there were no linkages to emit? Can that ever happen (a module with no functions and no globals?)


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

https://reviews.llvm.org/D104945