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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added a comment.

PTAL



================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:164
+  cast<MCSymbolWasm>(CurrentFnSym)->setSignature(Signature.get());
+  addSignature(std::move(Signature));
+
----------------
dschuff wrote:
> 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.
OK, I went further on this, and it's better. I think we should  go all the way and remove  the separate .param/.result but that requires changing all the tests, so it should be a separate CL.


Repository:
  rL LLVM

https://reviews.llvm.org/D52580