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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added a comment.

Generally looks good.  Yay for unblocking us from leaving experimental



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


================
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) {
----------------
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?


================
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);
 }
----------------
Why not set the type of FUNCTION in setSignature()?    And assert here maybe for sanity check?


================
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 =
----------------
I suppose this rename could be a separate NFC?   Although maybe not worth it.


================
Comment at: tools/obj2yaml/wasm2yaml.cpp:160
+        if (FunctionSig.Returns.size())
+          Sig.ReturnType = static_cast<uint32_t>(FunctionSig.Returns[0]);
+        for (const auto &ParamType : FunctionSig.Params)
----------------
Should we assert there that size() <= 1?


Repository:
  rL LLVM

https://reviews.llvm.org/D52580