[PATCH] D55149: [WebAssembly] Enforce assembler emits to streamer in order.

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel added inline comments.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:150
+      FunctionLocals,
+      Instructions,
+  } CurrentState;
----------------
aheejin wrote:
> clang-format this?
I did, it doesn't change :)


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:161
       : MCTargetAsmParser(Options, STI, MII), Parser(Parser),
-        Lexer(Parser.getLexer()) {
+        Lexer(Parser.getLexer()), CurrentState(FileStart), LastLabel(nullptr) {
     setAvailableFeatures(ComputeAvailableFeatures(STI.getFeatureBits()));
----------------
aheejin wrote:
> Nit: Can we use C++11-style in-class initialization for these two variables?
Sure.. I guess I was being conservative, not knowing what level of C++ I can use.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:400
+      // assembler and backend really don't share any common code, and
+      // this code parses the locals seperately.
       auto SymName = ExpectIdent();
----------------
aheejin wrote:
> Nit: Comment 80-col wrapping too, and maybe other parts as well
There's not much to wrap here, I wanted TODO to start on its own line.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:408
+        CurrentState = FunctionStart;
+      }
       auto Signature = make_unique<wasm::WasmSignature>();
----------------
aardappel wrote:
> This code is not super elegant, but kinda has to be this way because we have .functype which both introduces a function and ones which just declare external ones. Now if you misspel a label you'll fail to start a new function. 
> 
> More ideal would be split these, and have a .functype with no name that annotates the start of a function (and the assembler enforces only comes after a label) and a different one, e.g. .funcdecl, that does have its own name and is only used for external types. Not sure if such a refactoring is worth it though, after we just re-did all the tests with .functype.
Also note that in the past we decided that we like the assembler to be relatively "dumb", in terms of not needing to have a lot of structural knowledge about wasm. Turns out that is not possible with how streamers work, we need the assembler to enforce a certain level of structure.


================
Comment at: test/MC/WebAssembly/assembler-binary.ll:1
+; RUN: llc -filetype=asm -asm-verbose=false %s -o - | FileCheck -check-prefix=ASM %s
+; RUN: llc -filetype=asm -asm-verbose=false %s -o - | llvm-mc -triple=wasm32-unknown-unknown -filetype=asm -o - | FileCheck -check-prefix=ASM %s
----------------
aheejin wrote:
> Do we need to check all these asm and yaml outputs in this test? Wouldn't yaml output of function symbol related parts be sufficient?
Yes, but then when the yaml test breaks on e.g. the code bytes, it will be hard to track down which instruction is causing it. With the asm tests, those will break first so it will be easier to fix.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55149