[PATCH] D52580: Refactor WasmSignature and use it for MCSymbolWasm

[Derek Schuff via Phabricator via llvm-commits]

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


================
Comment at: include/llvm/Object/WasmTraits.h:27
   static wasm::WasmSignature getEmptyKey() {
-    return wasm::WasmSignature{{}, 1};
   }
----------------
sbc100 wrote:
> So we were just using 1 and 2 here as integers that happen to not be wasm value types?
Yeah, it looks that way.


================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp:238
 
-const char *llvm::WebAssembly::TypeToString(MVT Ty) {
-  switch (Ty.SimpleTy) {
-  case MVT::i32:
+const char *llvm::WebAssembly::TypeToString(wasm::ValType Ty) {
+  switch (Ty) {
----------------
sbc100 wrote:
> Maybe makes sense to move this to lib/BinaryFormat/Wasm.cpp now?   Since the other ToString methods for the bin format live there.   Although perhaps its good to have all the assembly string constants here?
> 
> Seems a little odd that the exact save strings are returned by the above function, maybe they can be unified somehow?
Yeah probably ExprType and ValType could be unified. There is maybe a little bit better organization or separation we could have between the raw enums in `BinaryFormat/Wasm.h` and ValType (and MVT), but I tried to keep that to a minimum for this CL. Also I don't know why the BinaryFormat functions are in namespace wasm and the other stuff is in namespace WebAssembly.

Maybe for now we can keep them here (so at least the strings in the data section can stay unified) and then fix the above in a follow-up.


================
Comment at: lib/Target/WebAssembly/MCTargetDesc/WebAssemblyTargetStreamer.cpp:152
     // Symbol already has its arguments and result set.
-    return;
-  }
-
-  SmallVector<wasm::ValType, 4> ValParams;
-  for (MVT Ty : Params)
-    ValParams.push_back(WebAssembly::toValType(Ty));
-
-  SmallVector<wasm::ValType, 1> ValResults;
-  for (MVT Ty : Results)
-    ValResults.push_back(WebAssembly::toValType(Ty));
-
-  WasmSym->setParams(std::move(ValParams));
-  WasmSym->setReturns(std::move(ValResults));
-  WasmSym->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
+    Symbol->setType(wasm::WASM_SYMBOL_TYPE_FUNCTION);
 }
----------------
sbc100 wrote:
> Why not set the type of FUNCTION in setSignature()?    And assert here maybe for sanity check?
For every other case it's set in EmitSymbolAttribute. Indirect function type directives are just weird because the symbol we are emitting corresponds to an undefined function. This is another thing we might rethink now that s2wasm is gone and we just have our own asm parser and printer to coordinate.


================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:164
+  cast<MCSymbolWasm>(CurrentFnSym)->setSignature(Signature.get());
+  addSignature(std::move(Signature));
+
----------------
sunfish wrote:
> With this patch as it is now, the WasmStreamer's emitParam no longer does anything, but the AsmStreamer does. This weakens the mental model of the AsmPrinter somewhat, because code like EmitFunctionBodyStart here is conceptually supposed to be "emitting assembly code" through directives, with those directives mapping to either emitting the actual text for the directives, or implementing the directives directly in binary. This mental model is useful for keeping the codegen to object file path in sync with the AsmParser to object file path.
> 
> Since I don't see a call to setSignature in the AsmParser to object file path, is it not currently supporting signatures with this patch?
Yeah I agree this is weird and should be fixed, although I decided not to take that any further for this CL. We can probably replace both of them with something else (i.e. different scheme of assembler directives) if that makes more sense.  I had actually forgotten that there was even AsmParser support for .param directives, since the assembly language is still in flux and we aren't actually using it anywhere right now.

And come to think of it we'll have to handle the ownership of signatures similarly (i.e. outside of the MCContext, perhaps on the AsmParser).

I think what we'll want to do is get rid of the separate .param and .result directives in favor of a full signature specifier; something more like what we have already for .functype (but make it able to handle multiple returns). I don't know of anywhere we'd need to use just params or just results. Not sure if it's better to do that in a followup CL (better for review, but would undo a bit of this CL) or pack it into this one.


================
Comment at: lib/Target/WebAssembly/WebAssemblyMCInstLower.cpp:112
   SmallVector<wasm::ValType, 4> Params;
-  GetSignature(Subtarget, Name, Returns, Params);
-
-  WasmSym->setReturns(std::move(Returns));
-  WasmSym->setParams(std::move(Params));
+  GetLibcallSignature(Subtarget, Name, Returns, Params);
+  auto Signature =
----------------
sbc100 wrote:
> I suppose this rename could be a separate NFC?   Although maybe not worth it.
done in rL343275


Repository:
  rL LLVM

https://reviews.llvm.org/D52580