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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin marked 4 inline comments as done.
aheejin added a comment.

In D88697#2307516 <https://reviews.llvm.org/D88697#2307516>, @dschuff wrote:

> 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?

Yes. For invokes the problem is, as you pointed out, we don't have final signatures there. For `emscripten_longjmp_jmpbuf` / `emscripten_longjmp`, the problem is not that we don't have the final signature there, but rather, we cannot have two functions with different signatures with the same name `emscripten_longjmp`. There are two occasions that we add a call to `emscripten_longjmp`, but one of them is calling it with an argument type `jmp_buf`. So we need two different signatures, and thus two different functions there. But `jmp_buf` is lowered to `i32` at this point so we can unify them.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:151-152
+  if (!Sig->Returns.empty())
+    for (auto VT : Sig->Returns)
+      Ret += getInvokeWrapperSig(VT);
+  else
----------------
tlively wrote:
> This naming scheme does not support multivalue functions. For example functions with signatures (i32) -> (i32, i32) and (i32, i32) -> (i32) both become invoke_iii and are indistinguishable. Would it make sense to error out if `Sig->Returns` has multiple elements?
I thought about this and initially tried to separate returns and params with `_`, but didn't end up doing it because
1. I'm not even sure if Emscripten EH supports multiple returns now.
2. Even if it does (or it will in future), I think making it a separate patch is better, because changing this will change the invoke interface currently shared with Emscripten.

Yeah, in the meantime, I think erroring out when people try to use multivalue functions with Emscripten EH/SjLj sounds like a good idea. Added the error message, and added 'lower-em-ehsjlj-multi-return.ll' to check this erroring out behavior.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:226
+      if (isEmscriptenEHSymbolName(Sym->getName())) {
+        if (EmSymbols.count(Sym))
+          continue;
----------------
dschuff wrote:
> I think you can just call `insert` unconditionally and ignore its return value in this case.
We still need to `continue` in case the symbol is already in the map. I changed the code to this. This does not ignore the return value but uses it to figure out whether we should `continue` or not. Did you mean this?
```
if (!EmSymbols.insert(Sym).second)                                       
  continue;
```


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.h:65
 
+  MCSymbolWasm *getMCSymbolForFunction(const Function *F, bool EnableEmEH,
+                                       wasm::WasmSignature *Sig);
----------------
dschuff wrote:
> this isn't an override, right? should it go after the functions that are part of the AsmPrinter implementation? 
It is used by `emitEndOfAsmFile` so I put it there, but yeah, I think moving its declaration down is better. It is still placed right before in `emitEndOfAsmFile` in cpp file; let me know if you think we should change that too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88697