[PATCH] D53842: [WebAssembly] Parsing missing directives to produce valid .o

[Wouter van Oortmerssen via Phabricator via llvm-commits]

aardappel marked an inline comment as done.
aardappel added inline comments.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:196
+        .Case("v128", {MVT::v16i8, wasm::WASM_TYPE_V128})
+        .Default({MVT::INVALID_SIMPLE_VALUE_TYPE, wasm::WASM_TYPE_NORESULT});
   }
----------------
dschuff wrote:
> This is pre-existing, so I don't want to hold up this CL, but I think this should only accept `v128` and not the MVT types. In the wasm spec there is only a `v128` value type (and hence params can only have this one type). In principle LLVM *shouldn't* need to know the MVT in its `.param` directive, although it's possible that something depends on that now. We should look into this separately. +cc @tlively 
I am fine modifying this either way, but yes, for MVT we don't have a generic type, so it is currently returning v16i8 which is bad.


================
Comment at: lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:351
       // "label, at function"
-      if (!(IsNext(AsmToken::Identifier) && IsNext(AsmToken::Comma) &&
-            IsNext(AsmToken::At) && Lexer.is(AsmToken::Identifier)))
+      if (!Lexer.is(AsmToken::Identifier))
+        return Error("Expected label after .type directive, got: ",
----------------
sbc100 wrote:
> dschuff wrote:
> > It just occurred to me: do we need to make this stateful like this? Should a `.type` directive create a symbol/type association regardless of where it is with respect to the actual label? It looks like the ELF Asm parser (`ParseDirectiveType`) just looks up the symbol (creating if necessary) and emits a symbol attribute to the streamer (which is what actually manipulates the symbol). Maybe we could use this pattern in wasm too, to make asm vs object more uniform?
> This sounds good to me.  Than we could avoid LastTypeOfSymbol and GlobalTypeFixups completely?
Made it less stateful using getOrCreateSymbol.


Repository:
  rL LLVM

https://reviews.llvm.org/D53842