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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

D147852 <https://reviews.llvm.org/D147852> landed, so please rebase onto main and resolve conflicts.



================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:63
+    return OperandStack{Sig.Returns.begin(), Sig.Returns.end()};
+    ;
+  }
----------------
Stray `;`


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:437
       return true;
   } else if (Name == "catch") {
+    Unreachable = false;
----------------
D147852 moves this clause into `else if (Name == "end_block" || Name == "end_loop" || Name == "end_if" || ...`, so please resolve the conflict


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:187
+                                   std::to_string(BrLevel) + " (max " +
+                                   std::to_string(BrStack.size()) + ")");
+  const llvm::WebAssemblyAsmOperandFrame &Frame =
----------------
HerrCai0907 wrote:
> 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?
Yes, the function body is also a block you can `br` to.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:226-227
+  else {
+    if (Unreachable)
+      Stack = BrStack.pop_back_val().applySignature();
+    else
----------------
HerrCai0907 wrote:
> 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
> 
> ```
(The comments have moved so it's hard to track what code this was about, but it was about this code)
```
    // In some cases (unreachable / br), `Stack` will be invalid and need to be
    // recovered.
    if (Unreachable)
      Stack = BrStack.pop_back_val().applySignature();
    else
      BrStack.pop_back();
```

I think we already check this case? (The checker/parser had some bugs but some of them were fixed in D147852 and D147881, so please check them too) Can you give me an example that the current checker/parser misses on this case?

Also it's a little confusing that how much of the functionalities this CL adds overlaps with the existing ones. Currently we do some type checking in the parser too, which is hard to remove because the parser needs to check some basic things to parse. The CL header claims it is adding `br` checking, and I don't think we do `br` checking elsewhere, so I think it's a good thing to add, but this CL adds a lot more things. Can we possibly simplify this CL by adding only the missing checks?


================
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;
----------------
HerrCai0907 wrote:
> 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.
Yeah a separate formatting CL is fine, but I wonder why your `arc diff` unconditionally formats it. There should be a way or an option not to do it. My `arc diff` asks me, doesn't unconditionally formats files, when submitting a CL, and also it only asks me if the lines I modified are not formatted and doesn't check other lines. What's your `arc` version?


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:482-484
+    if (Name == "if" &&
+        checkBlockIn(WebAssemblyAsmOperandFrame::FrameType::If, ErrorLoc, Inst))
+      return true;
----------------
aheejin wrote:
> While is this here, while other block-like instructions (`block`, `try`, and `loop`) are checked separately (line 391-402) above?
Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147504