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

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked 4 inline comments as done and an inline comment as not done.
aardappel added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:1361
+  assert(false && "mnemonic not found");
+  return StringRef();
+}
----------------
dschuff wrote:
> 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.
No, it is used by the big long if-else chain of the main type checker above, so it is definitely a possible performance issue.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:243
+  // Type Checker:
+  struct TypeStack {
+    SmallVector<wasm::ValType, 8> Stack;
----------------
dschuff wrote:
> 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`. 
That was to prepare for having copies of this stack in `TypedLabels`, but that so far isn't implemented. I can remove it for now.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:1012
 
+  void DumpTypeStack(Twine Msg) {
+    LLVM_DEBUG({
----------------
dschuff wrote:
> 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`
Yes, wasn't aware of it.. `LLVM_DUMP_METHOD` by itself doesn't do anything, but there is the practice to create `dump`  methods on types rather than what I did here.. I could do that on `TypeStack` that I am about the remove :) But in all, the existing code is simpler.


================
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") {
----------------
dschuff wrote:
> Is this right? function signatures for indirect calls aren't checked until runtime?
I'm not sure what you're asking. For the assembler, the instruction specifies a signature inline, so it is checked against the stack at.. assembly time. For Wasm in general, the i32 on the stack can be any old integer, and thus, any function with any signature, so that is checked at runtime.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:357
+void WebAssemblyAsmPrinter::emitEndOfAsmFile(Module &M) {
+  emitExternalDecls(M);
 
----------------
dschuff wrote:
> 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?)
yes, this was the location where the code was originally run from, so I put it here to make sure it runs at least once. It may well be that it can never happen, but I'd rather protect against that.


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

https://reviews.llvm.org/D104945