[PATCH] D44329: [WebAssembly] Added initial AsmParser implementation.

[Derek Schuff via Phabricator via llvm-commits]

dschuff added a comment.

looking pretty good overall, here are a couple of high-level comments.



================
Comment at: lib/MC/MCExpr.cpp:402
 }

 bool MCExpr::evaluateAsAbsolute(int64_t &Res, const MCAssembler &Asm) const {
   return evaluateAsAbsolute(Res, &Asm, nullptr, nullptr);
----------------
Wow, is it just me or did the diff engine really screw this file up :/


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:215
+        .Case("f64", WebAssembly::F64_0)
+        .Case("i8x16", WebAssembly::V16I8_0)
+        .Case("i16x8", WebAssembly::V8I16_0)
----------------
There are a couple of confusing things to me about this. First, the SIMD proposal has only one value type ([[ https://github.com/WebAssembly/simd/blob/master/proposals/simd/SIMD.md#simd-value-type | v128 ]]) and the wasm backend has a corresponding register class ([[ https://github.com/llvm-mirror/llvm/blob/master/lib/Target/WebAssembly/WebAssemblyRegisterInfo.td#L61 | V128 ]]) into which all the SIMD MVTs (v4f32 et al) can go. But in the assembly language the scalar instructions use the value type name in their mnemonic and the SIMD instructions use the lane division interpretation names. So that's a bit of inherent weirdness that will have to be reflected in this code somewhere. But the other thing is that this function is named `ParseRegType` which to me would indicate the type of register (i.e. register class which in C++ code would be e.g. `WebAssembly::I32RegClass`), but what it returns is actually a physical register number. What we maybe actually want is to reflect either a wasm value type (although it looks like that's maybe not actually even reflected directly in the MIR, just in the binary format) or an MVT (e.g. `MVT::i32` or `MVT::v4f32`). 


... ok actually looking at the way these reg types are used I now actually understand the comment you wrote in `WebAssemblyOperand::RegOp`.  Not sure I have a better idea right now. I guess it's because the generated matcher expects the reg operand to be one of the physical registers and validates that?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:249
+        return Error("Cannot parse stack register index: ", StackRegTok);
+      unsigned RegNo = static_cast<unsigned>(ParsedRegNo);
+      if (StackOp == "push") {
----------------
Up until now, the stack "reg" numbers on the "$push" and "$pop" tokens were really just annotations for readability, (because of course the operands push and pop on the stack in a defined order). So far nothing has actually depended on those notations, and this seems to be adding that dependence. Not that we absolutely can't do that; but it would be nice if we didn't have to. It looks like it might make this code more awkward though, because it appears that we are just getting the lexer tokens in order, and we'd maybe have to buffer them or something before processing them in right-to-left order. Am I understanding this correctly?


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:291
+    Parser.Lex();
+    return false;
+  }
----------------
jgravelle-google wrote:
> Wait, is truthiness here used to represent failure? That's... confusing.
> Oh, that's overridden from `MCTargetAsmParser::ParseInstruction`, cool.
There are a lot of places in LLVM backends that return true for failure and false for success ¯\_(ツ)_/¯
edit: actually there doesn't appear to be a failure path in this function, but its caller below clearly expects that possibility?


https://reviews.llvm.org/D44329