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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

This will have conflicts with D147852 <https://reviews.llvm.org/D147852>, so depending on which lands first, the other needs to resolve them. D147852 <https://reviews.llvm.org/D147852> doesn't intend to add new functionalities (like `br` checking here)  though; it just handles other block-ending instructions in the same way as `end_block`.



================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:54-57
+WebAssemblyAsmOperandFrame::WebAssemblyAsmOperandFrame(
+    FrameType FrameType, const SmallVectorImpl<wasm::ValType> &Stack,
+    const wasm::WasmSignature &Signature)
+    : Type(FrameType), EnterStack(Stack.begin(), Stack.end()), Sig(Signature) {}
----------------
I think you can move this to the header, given that this doesn't have a body


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:63
+  case FrameType::Loop:
+    // https://webassembly.github.io/spec/core/valid/instructions.html#xref-syntax-instructions-syntax-instr-control-mathsf-loop-xref-syntax-instructions-syntax-blocktype-mathit-blocktype-xref-syntax-instructions-syntax-instr-mathit-instr-ast-xref-syntax-instructions-syntax-instr-control-mathsf-end
+    return OperandStack{Sig.Params.begin(), Sig.Params.end()};
----------------
I don't think we need to link the formal spec here; it's assumed the implementer and the reader of this file either knows or is able to look for it. We don't link it elsewhere in the file either.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:167-168
+static std::optional<std::string>
+checkStackTop(SmallVectorImpl<wasm::ValType> const &ExpectedStackTop,
+              SmallVectorImpl<wasm::ValType> const &Got) {
+  for (size_t I = 0; I < ExpectedStackTop.size(); I++) {
----------------



================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:187
+                                   std::to_string(BrLevel) + " (max " +
+                                   std::to_string(BrStack.size()) + ")");
+  const llvm::WebAssemblyAsmOperandFrame &Frame =
----------------
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.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:192
+      checkStackTop(Frame.getResultType(), Stack);
+  if (StackCheckResult.has_value())
+    return typeError(ErrorLoc,
----------------



================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:202
+  BrStack.push_back(WebAssemblyAsmOperandFrame{Type, Stack, LastSig});
+  // check signature
+  WebAssemblyAsmOperandFrame::OperandStack EnterStack =
----------------



================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:204
+  WebAssemblyAsmOperandFrame::OperandStack EnterStack =
+      BrStack.back().getEnterStack();
+  for (wasm::ValType VT : llvm::reverse(LastSig.Params))
----------------
TODO aheejin copy? do we use this later again?


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:218
 
-  for (size_t i = 0; i < LastSig.Returns.size(); i++) {
-    auto EVT = LastSig.Returns[i];
-    auto PVT = Stack[Stack.size() - LastSig.Returns.size() + i];
-    if (PVT != EVT)
-      return typeError(
-          ErrorLoc, StringRef("end got ") + WebAssembly::typeToString(PVT) +
-                        ", expected " + WebAssembly::typeToString(EVT));
+  if (StackCheckResult.has_value())
+    return typeError(ErrorLoc,
----------------



================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:221-222
+                     StringRef{"end "} + StringRef{StackCheckResult.value()});
+  // in some cases (unreachable / br), `Stack` will be invalid and need to be
+  // recover
+  if (RecoverEnter)
----------------



================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:226-227
+  else {
+    if (Unreachable)
+      Stack = BrStack.pop_back_val().applySignature();
+    else
----------------
Why is this and what does this code do?


================
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;
----------------
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.


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:279
     return typeError(ErrorLoc, StringRef("symbol ") + WasmSym->getName() +
-                                    " missing .globaltype");
+                                   " missing .globaltype");
   }
----------------
Please revert unrelated clang-format changes 


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:417-420
+    if (popType(ErrorLoc, wasm::ValType::I32))
+      return true;
+    if (checkSig(ErrorLoc, LastSig))
+      return true;
----------------
Please revert unrelated clang-format changes 


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.cpp:433-434
                                                        " missing .functype");
-    if (checkSig(ErrorLoc, *Sig)) return true;
+    if (checkSig(ErrorLoc, *Sig))
+      return true;
     if (Name == "return_call" && endOfFunction(ErrorLoc))
----------------
Please revert unrelated clang-format changes 


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


================
Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmTypeCheck.h:41
+                             const wasm::WasmSignature &Signature);
+  OperandStack const &getEnterStack() const { return EnterStack; }
+  OperandStack getResultType() const;
----------------



================
Comment at: llvm/test/MC/WebAssembly/basic-assembly.s:84-88
+    drop
+    drop
+    i32.const 1
+    if                   # void
+    i32.const 1
----------------
Why is this change necessary?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147504