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

[Derek Schuff via Phabricator via llvm-commits]

dschuff 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
----------------
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.


================
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) {
----------------
This can go inside the `pruneUnreachableresumes` conditional since it's only used there.


================
Comment at: lib/CodeGen/TargetPassConfig.cpp:660
+    // stops at every call frame with a landing pad.
+    addPass(createDwarfEHPass(false));
     break;
----------------
this could be `addPass(createDwarfEHPass(/*PruneUnreachableResumes=*/false)` to make it more greppable.


================
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
----------------
I assume the actual value of the index is a literal in the generated IR, and is assigned by this pass?


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:37
+//
+//    The old landingpad instruction will be deleted in instruction selection
+//    phase.
----------------
why not just delete it here from LLVM IR now that it's dead? or does it also get lowered somehow?


================
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
----------------
->represent a thrown exception


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:107
+// retrieve the selector after it returns.
+//
+//===----------------------------------------------------------------------===//
----------------
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.


================
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
----------------
Could these `getDeclaration`s, `getOrInsertFunction`s, GlobalVariable fields etc be moved to `doInitialization`? Or maybe all except the one that requires the TargetMachine?


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:274
+  for (BasicBlock *Pred : predecessors(BB)) {
+    auto *II = dyn_cast<InvokeInst>(Pred->getTerminator());
+    assert(II && "Landingpad's predecessor does not end with invoke");
----------------
Could just use cast instead of dyn_cast and use its builtin assert.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:285
+  // __wasm_lpad_context.lpad_index = index;
+  IRB.CreateStore(IRB.getInt32(Index), LPadIndexField, true);
+
----------------
could be `/*isVolatile=*/true` for readability. For that matter, why volatile?


Repository:
  rL LLVM

https://reviews.llvm.org/D43746