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

[Congcong Cai via Phabricator via llvm-commits]

HerrCai0907 planned changes to this revision.
HerrCai0907 marked an inline comment as done.
HerrCai0907 added a comment.

I will change those point and add comment also in code.



================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:187
+                                   std::to_string(BrLevel) + " (max " +
+                                   std::to_string(BrStack.size()) + ")");
+  const llvm::WebAssemblyAsmOperandFrame &Frame =
----------------
aheejin wrote:
> Shouldn't we do -1? If not, the message can be something like 
> ```
> br: invalid depth 3 (max 3)
> ```
> 
> But -1 also has a problem, because when `BrStack` is empty it will print something like 
> ```
> br: invalid depth 0 (max -1)
> ```
> So if we want to print `(max N)`, it would be better to handle that case smoothly too.
I also think it is `-1`, but when I use MDN's [web runner](https://developer.mozilla.org/en-US/docs/WebAssembly/Reference/Control_flow/br) for following code
```
(module
  (import "console" "log" (func $log (param i32)))

  (func $hhh
    i32.const 1
    call $log
    br 0
    i32.const 2
    call $log
  )

  (start 1) ;; run the first function automatically
)
``
It outputs `1`
I am not sure whether func body is a block which can also `br` to?


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:204
+  WebAssemblyAsmOperandFrame::OperandStack EnterStack =
+      BrStack.back().getEnterStack();
+  for (wasm::ValType VT : llvm::reverse(LastSig.Params))
----------------
aheejin wrote:
> aheejin wrote:
> > TODO aheejin copy? do we use this later again?
> Sorry, forgot to remove my own memo here, and Phabricator doesn't give a way to delete inline comments... Please ignore the message above. By the way, a question: this looks copying the stack intentionally not to modify `BrStack.back().EnterStack`, correct?
correct. here just checks whether the parameters of label are fulfilled.
e.g. 
```
i32.const 1
block (i32, i32) -> (i32)
end_block
```


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:226-227
+  else {
+    if (Unreachable)
+      Stack = BrStack.pop_back_val().applySignature();
+    else
----------------
aheejin wrote:
> Why is this and what does this code do?
consider this case
```
i32.const 1
if  // store op stack
    return
    // here can do everything because of stack-polymorphic
else
    // here stack should be recovered as stored
end_if

```


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:236-240
 bool WebAssemblyAsmTypeCheck::checkSig(SMLoc ErrorLoc,
-                                       const wasm::WasmSignature& Sig) {
+                                       const wasm::WasmSignature &Sig) {
   for (auto VT : llvm::reverse(Sig.Params))
-    if (popType(ErrorLoc, VT)) return true;
+    if (popType(ErrorLoc, VT))
+      return true;
----------------
aheejin wrote:
> I think you applied `clang-format` to the whole file. While we try to keep files clang-formatted, I think it'd be better not to mix this with other functionality changes. Can you revert this?
> 
> Tip: `git clang-format main` will clang-format only the changed lines, not the whole file. You can replace `main` with another branch or a commit if necessary.
Could it possible to submit a format patch before it?
Actually my arc always does format when I use arc diff.


================
Comment at: llvm/test/MC/WebAssembly/basic-assembly.s:84-88
+    drop
+    drop
+    i32.const 1
+    if                   # void
+    i32.const 1
----------------
aheejin wrote:
> Why is this change necessary?
The old file is invalid but not be detected. (The op stack of true and false branch in if block are not balanced)
The reason I drop and push i32 is to avoid coupling in each checking block.
drop all operands in "switch" and re-introduce for "if"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147504