[PATCH] D43746: [WebAssembly] Add Wasm exception handling prepare pass

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: include/llvm/IR/IntrinsicsWebAssembly.td:38
 
+// This intrinsic has a functionality of wasm catch / if_except / else / end
+// instructions combined. Wasm catch instruction returns a caught wasm
----------------
dschuff wrote:
> This description is a little confusing. Maybe it should start with a high-level description of its semantics in terms of LLVM IR (i.e. without reference to the wasm primitives that implement it), and then after that put this description of how it's lowered.
This part will go away when we switch to windows EH instructions.


================
Comment at: lib/CodeGen/DwarfEHPrepare.cpp:209
 
-  size_t ResumesLeft = pruneUnreachableResumes(Fn, Resumes, CleanupLPads);
-  if (ResumesLeft == 0)
-    return true; // We pruned them all.
+  size_t ResumesLeft = Resumes.size();
+  if (PruneUnreachableResumes) {
----------------
dschuff wrote:
> This can go inside the `pruneUnreachableresumes` conditional since it's only used there.
After we switch to windows EH, we don't need this anymore.


================
Comment at: lib/CodeGen/TargetPassConfig.cpp:660
+    // stops at every call frame with a landing pad.
+    addPass(createDwarfEHPass(false));
     break;
----------------
dschuff wrote:
> this could be `addPass(createDwarfEHPass(/*PruneUnreachableResumes=*/false)` to make it more greppable.
After we switch to windows EH, we don't need this anymore.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:16
+// 1. Insert a call to wasm.eh.landingpad.index() before every invoke
+//    instruction. Each landing pad has its own index starting from 0, and the
+//    argument to wasm.eh.landingpad.index() is the index of the landing pad
----------------
dschuff wrote:
> I assume the actual value of the index is a literal in the generated IR, and is assigned by this pass?
Yes, it is generated by this pass, assigning an integer from 0 when traversing eh pads.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:37
+//
+//    The old landingpad instruction will be deleted in instruction selection
+//    phase.
----------------
dschuff wrote:
> why not just delete it here from LLVM IR now that it's dead? or does it also get lowered somehow?
Landingpad instructions are lowered in [[ https://github.com/llvm-mirror/llvm/blob/b273503311ade5f98f2879195ed9c5655c3fff1b/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp#L2468-L2517 | `SelectionDAG::visitLandingPad` ]], so we needed that by then. But anyway, after we switch to winEH we are not gonna have `landingpad`s anymore; instead we are gonna have `catchpad`s and `cleanuppad`s.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:54
+// the current stage. WebAssembly catch instruction returns an exception object
+// whose type is except_ref type, an opaque type to represent an exception in
+// WebAssembly. This except_ref type object contains the original i8* exception
----------------
dschuff wrote:
> ->represent a thrown exception
Done.. but this part is gonna change anyway.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:107
+// retrieve the selector after it returns.
+//
+//===----------------------------------------------------------------------===//
----------------
dschuff wrote:
> maybe put a TODO here to make a way to model the exception_ref in C++ so we can move  more of the generated code into libraries.
If we switch to winEH, we don't need it for now; will do it once we get to use except_ref later.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:195
+  // it
+  CatchF = Intrinsic::getDeclaration(&M, Intrinsic::wasm_catch_extract);
+  // wasm.eh.landingpad.index() intrinsic, which is to specify landingpad index
----------------
dschuff wrote:
> Could these `getDeclaration`s, `getOrInsertFunction`s, GlobalVariable fields etc be moved to `doInitialization`? Or maybe all except the one that requires the TargetMachine?
Done. I initially put them in `runOnFunction` because I didn't want to insert them if the program does not use exceptions thus has no landing pad. But they are gonna be deleted anyway at the end if unused, so I guess it's fine to move.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:285
+  // __wasm_lpad_context.lpad_index = index;
+  IRB.CreateStore(IRB.getInt32(Index), LPadIndexField, true);
+
----------------
dschuff wrote:
> could be `/*isVolatile=*/true` for readability. For that matter, why volatile?
I didn't want to make them optimized away, so I thought setting `isVolatile` to true would be safer, no?


Repository:
  rL LLVM

https://reviews.llvm.org/D43746