[PATCH] D88697: [WebAssembly] Rename Emscripten EH functions

[Derek Schuff via Phabricator via llvm-commits]

dschuff added a subscriber: aardappel.
dschuff added a comment.

So, just to be sure I understand the architecture here:
Previously we use separate `longjup_jmpbuf`/`longjmp` (and `__invoke_{$LLVMSIG}` because the lowering pass runs on IR and the IR type system's rules apply (and we don't have wasm signatures there). Now we can get wasm function signatures at AsmPrinter::emitEndOfAsmFile and the only thing that needs to be done is rename them?



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:118
-      // imports. so fix the names and the tests, or rethink how import
-      // delcarations work in asm files.
       getTargetStreamer()->emitFunctionType(Sym);
----------------
aheejin wrote:
> This comment doesn't sound relevant now. This was added in D52580 by @dschuff, and then the line below this comment was this:
> ```
> getTargetStreamer()->emitIndirectFunctionType(Sym);
> ```
> 
> I don't know the full history, but the comment seemed to be somehow related to indirect function types, but it seems we are using this for all functions types, so we don't need this FIXME anymore..?
In the meantime @aardappel did rethink how import declarations work in asm files (and that's why we got the `.functype` directive in https://reviews.llvm.org/D54652). And he also fixed the huge pile of tests that this comment refers to. so removing this FIXME LGTM.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:121
 
-      if (TM.getTargetTriple().isOSBinFormatWasm() &&
-          F.hasFnAttribute("wasm-import-module")) {
----------------
aheejin wrote:
> I guess now `isOSBinFormatWasm()` is always true? So removed it
LGTM, we don't support wasm-in-ELF anymore.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:155
+    Ret += 'v';
+  if (!Sig->Params.empty()) {
+    // Invoke wrappers' first argument is a pointer to the original function, so
----------------
I don' think this check is necessary, empty params just won't trip the loop.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:226
+      if (isEmscriptenEHSymbolName(Sym->getName())) {
+        if (EmSymbols.count(Sym))
+          continue;
----------------
I think you can just call `insert` unconditionally and ignore its return value in this case.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h:65
 
+  MCSymbolWasm *getMCSymbolForFunction(const Function *F, bool EnableEmEH,
+                                       wasm::WasmSignature *Sig);
----------------
this isn't an override, right? should it go after the functions that are part of the AsmPrinter implementation? 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88697/new/

https://reviews.llvm.org/D88697