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

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:63
+// they are not generated for now. catch_all catches all exceptions including
+// foreign exceptions. We turn catchpads into catch (C++ tag) and cleanuppads
+// into catch_all, with one exception: cleanuppad with a call to
----------------
probably worth making this "foreign exceptions (e.g. JavaScript)" since that's the expected case.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:69
+// * Background: Direct personality function call
+// In WebAssembly EH, the VM is responsible for unwinding stack once an
+// exception is thrown. After stack is unwound, the control flow is transfered
----------------
s/"stack"/"the stack" in most of the uses in this comment block.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:217
+      CatchPads.push_back(&BB);
+    if (isa<CleanupPadInst>(BB.getFirstNonPHI()))
+      CleanupPads.push_back(&BB);
----------------
could be `else if`


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:298
+  // Pseudocode: __wasm_lpad_context.lpad_index = index;
+  IRB.CreateStore(IRB.getInt32(Index), LPadIndexField, /*isVolatile=*/true);
+
----------------
I still don't see why these stores need to be volatile. They store to a global so they shouldn't be touched before linking, and when the optimizer can see the uses, then it should do the right thing anyway?


================
Comment at: lib/CodeGen/WinEHPrepare.cpp:54
+    "demote-catchswitch-only", cl::Hidden,
+    cl::desc("Demote catchswitch BBs only (for wasm EH)"), cl::init(false));
+
----------------
Why do catchswitch phis need to be removed if we don't outline any funclets?


Repository:
  rL LLVM

https://reviews.llvm.org/D43746