[PATCH] D46803: [WebAssembly] Add WebAssemblyExceptionPrepare pass

Heejin Ahn via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 19 15:25:33 PDT 2018


aheejin added inline comments.


================
Comment at: lib/Target/WebAssembly/WebAssemblyExceptionPrepare.cpp:27
 namespace {
 class WebAssemblyExceptionPrepare final : public MachineFunctionPass {
   StringRef getPassName() const override {
----------------
dschuff wrote:
> I didn't think about the confusing-ness of this name when we put it in originally; maybe we could call it 'WebAssemblyMachineEHPrepare' or 'WebAssemblyLateEHPrepare'
Hmm, not to be confused with `WasmEHPrepare`? Yeah, that makes sense.. I'll go with `WebAssemblyLateEHPrepare` because 'Machine' sounds like it has its same or similar counterpart in LLVM IR passes. (like, `LoopInfo` vs `MachineLoopInfo`)


================
Comment at: lib/Target/WebAssembly/WebAssemblyExceptionPrepare.cpp:110
+  Changed |= addRethrows(MF);
   if (!MF.getFunction().hasPersonalityFn())
+    return Changed;
----------------
dschuff wrote:
> So a function can rethrow but not have a personality function?
A function has its associated personality function when it has an EH pad (landingpad or catchpad/cleanuppad). Functions that call `__cxa_rethrow()` can rethrow to the caller, in which case the function doesn't have an EH pad. I didn't want to run all the functions below needlessly in that case.


================
Comment at: lib/Target/WebAssembly/WebAssemblyExceptionPrepare.cpp:212
+// Add a 'rethrow' instruction after __cxa_rethrow() call
+bool WebAssemblyExceptionPrepare::addRethrows(MachineFunction &MF) {
+  bool Changed = false;
----------------
dschuff wrote:
> Would it make more sense to just have the rethrow inside of cxa_rethrow (at least if the destination is the caller)? It might save some on code size? I guess we would just have to put an 'unreachable' after the call and it wouldn't matter; or is there some other reason?
That is the spec question. The current implementation is based on [[ https://github.com/WebAssembly/exception-handling/blob/master/proposals/Exceptions.md | the first proposal ]], in which `rethrow` can only be placed within a catch block. We can change this later if that is shown to save code size.

I'll do some experiments on that once I'm able to run things end-to-end, but I actually doubt if that would be saving much, because we can save one `rethrow` instruction if we put that within a function, but that also means we have to pass a first-class exception argument to `__cxa_rethrow()`, and to put that argument on the wasm stack we have to consume one more instruction anyway. And also, if we use first-class exception objects, now every `catch` clause has to have that `if_except` ~ `else` ~ `end` instruction sequence too, which also contributes to the code size.

Anyway, I'm not saying I'm against it though. I'm saying just the current implementation is based on the first proposal :$


Repository:
  rL LLVM

https://reviews.llvm.org/D46803





More information about the llvm-commits mailing list