[PATCH] D147504: [WebAssembly] `AsmTypeCheck` support to br instr

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

This looks pretty good!



================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:206-207
+  for (wasm::ValType VT : llvm::reverse(LastSig.Params)) {
+    WebAssemblyAsmOperandFrame::OperandStack EnterStack =
+        BrStack.back().getEnterStack();
+    if (popType(EnterStack, ErrorLoc, VT))
----------------
Should this be outside the loop over the params? Otherwise we start with the same stack when checking each parameter, right?


================
Comment at: llvm/test/MC/WebAssembly/type-checker-control-flow.s:83
+# CHECK-NEXT: end_function
\ No newline at end of file

----------------
It would be good to add a newline here.


================
Comment at: llvm/test/MC/WebAssembly/type-checker-errors.s:524-525
+      i32.const 0
+# CHECK: :[[@LINE+1]]:7: error: br got [..., i32], expected [...]
+      br 1
+    else
----------------
This is actually valid. The rule for `br` is that if the label has type `[t*]`, the stack before the `br` can be `[t1* t*]` for any `t1*`. The `br` implicitly drops everything in `t1*`.

See https://webassembly.github.io/spec/core/valid/instructions.html#xref-syntax-instructions-syntax-instr-control-mathsf-br-l


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147504