[PATCH] D57134: [WebAssembly] Exception handling: Switch to the new proposal

[Derek Schuff via Phabricator via llvm-commits]

dschuff added a comment.

haven't quite gotten through it but here's what I have so far. Looking pretty good overall though.



================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:252
+  ExtractExnF =
+      Intrinsic::getDeclaration(&M, Intrinsic::wasm_extract_exception);
+
----------------
aheejin wrote:
> If an intrinsic has a 'token' argument, it does not go through isel well.
For windows, do those just get lowered away in EHPrepare too then?


================
Comment at: lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:314
+    // readability.
+    if (!WasmKeepRegisters)
+      break;
----------------
Shouldn't this be an assert? i.e. if WasmKeepRegisters is disabled this should have been lowered away, right?


================
Comment at: lib/Target/WebAssembly/WebAssemblyInstrControl.td:144
 let isTerminator = 1, hasCtrlDep = 1, isBarrier = 1 in {
-defm THROW_I32 : I<(outs), (ins event_op:$tag, I32:$val),
-                   (outs), (ins event_op:$tag),
-                   [(WebAssemblythrow (WebAssemblywrapper texternalsym:$tag),
-                                      I32:$val)],
-                   "throw   \t$tag, $val", "throw   \t$tag",
-                   0x08>;
-defm THROW_I64 : I<(outs), (ins event_op:$tag, I64:$val),
-                   (outs), (ins event_op:$tag),
-                   [(WebAssemblythrow (WebAssemblywrapper texternalsym:$tag),
-                                      I64:$val)],
-                   "throw   \t$tag, $val", "throw   \t$tag",
-                   0x08>;
-defm RETHROW : NRI<(outs), (ins bb_op:$dst), [], "rethrow \t$dst", 0x09>;
-let isCodeGenOnly = 1 in
-// This is used when the destination for rethrow is the caller function. This
-// will be converted to a rethrow in CFGStackify.
-defm RETHROW_TO_CALLER : NRI<(outs), (ins), [], "rethrow">;
+defm THROW : I<(outs), (ins event_op:$tag, variable_ops),
+               (outs), (ins event_op:$tag),
----------------
dumb question: why is throw variadic now?


================
Comment at: test/CodeGen/WebAssembly/cfg-stackify-eh.ll:24
+; CHECK:   block except_ref
+; CHECK:   br_on_exn 0, __cpp_exception at EVENT, $[[EXCEPT_REF]]
+; CHECK:   rethrow
----------------
Is there any way to get a nice annotation for `br_on_exn` like we have for br/rethrow?
actually for that matter, I notice that we also don't have basic block labels (e.g. `.LBB0`) in these tests anymore. I assume they are still there but you are just not `CHECK`ing for them?


================
Comment at: test/MC/WebAssembly/annotations.s:1
+# RUN: llvm-mc -triple=wasm32-unknown-unknown -mattr=+unimplemented-simd128,+nontrapping-fptoint,+exception-handling < %s | FileCheck %s
+
----------------
are simd128 and nontrapping-fptoint needed here?


================
Comment at: test/MC/WebAssembly/annotations.s:9
+  .type    test_annotation, at function
+test_annotation:
+  .functype   test_annotation () -> ()
----------------
Could add `br_on_exn` to this file too. (and/or the one below)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57134