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

[Heejin Ahn via Phabricator via llvm-commits]

aheejin marked 13 inline comments as done.
aheejin added a comment.

- Added some in-line comments for easy reading.
- This has gotten pretty big and looks not very easy to review, but this was the only way if I want to switch things and make tests working. :( If it looks to big, I guess the only alternative is remove everything EH-related from the codebase first and put things bit by bit again.



================
Comment at: include/llvm/CodeGen/WasmEHFuncInfo.h:28
 struct WasmEHFuncInfo {
-  // When there is an entry <A, B>, if an exception is not caught by A, it
-  // should next unwind to the EH pad B.
----------------
These `EHPadUnwindMap` related stuff has been deleted because we stop at every catchpads now, so we don't need to worry about where we unwind next after an exception is not caught by a catchpad.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:252
+  ExtractExnF =
+      Intrinsic::getDeclaration(&M, Intrinsic::wasm_extract_exception);
+
----------------
If an intrinsic has a 'token' argument, it does not go through isel well.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:266
-  // wasm.catch() intinsic, which will be lowered to wasm 'catch' instruction.
-  CatchF = Intrinsic::getDeclaration(&M, Intrinsic::wasm_catch);
   // wasm.landingpad.index() intrinsic, which is to specify landingpad index
----------------
We don't need `catch` intrinsic anymore; we don't lower them to `catch` instruction in isel but we add all `catch` instructions in LateEHPrepare.


================
Comment at: lib/CodeGen/WasmEHPrepare.cpp:310
   // need to call personality function because we don't need a selector.
-  if (FPI->getNumArgOperands() == 0 ||
-      (FPI->getNumArgOperands() == 1 &&
-       cast<Constant>(FPI->getArgOperand(0))->isNullValue())) {
+  if (!NeedLSDA) {
     if (GetSelectorCI) {
----------------
This condition has not changed; this is just to make condition expression cleaner.


================
Comment at: lib/Target/WebAssembly/InstPrinter/WebAssemblyInstPrinter.cpp:145
+        printAnnotation(OS, "down to catch" + utostr(EHPadStack.back()));
       }
 
----------------
Brought the special handling for `rethrow` out of the for loop below, because `rethrow` does not have 'depth' operand anymore, and the loop below is looking for immediate depth operands.


================
Comment at: lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:114
-    // Even if a rethrow takes a BB argument, it is not a branch
-    if (!WebAssembly::isRethrow(MI))
-      for (MachineOperand &MO : MI.explicit_operands())
----------------
Rethrow now does not have a depth argument, so deleted this.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1076
+    if (Tag != CPP_EXCEPTION)
       llvm_unreachable("Invalid tag!");
+    const TargetLowering &TLI = DAG.getTargetLoweringInfo();
----------------
Early exit, because we now only have `CPP_EXCEPTION` case and that's not gonna change soon.


================
Comment at: lib/Target/WebAssembly/WebAssemblyISelLowering.h:99
   SDValue LowerCopyToReg(SDValue Op, SelectionDAG &DAG) const;
-  SDValue LowerINTRINSIC_WO_CHAIN(SDValue Op, SelectionDAG &DAG) const;
-  SDValue LowerINTRINSIC_VOID(SDValue Op, SelectionDAG &DAG) const;
+  SDValue LowerIntrinsic(SDValue Op, SelectionDAG &DAG) const;
   SDValue LowerSIGN_EXTEND_INREG(SDValue Op, SelectionDAG &DAG) const;
----------------
Merge all intrinsic lowering functions into one, because there's no point in keeping them separate by whether they have chain or not, and we have only a couple of them anyway. 


================
Comment at: lib/Target/WebAssembly/WebAssemblyLateEHPrepare.cpp:389
-//   call @std::terminate()
-//   unreachable
-bool WebAssemblyLateEHPrepare::addCatchAllTerminatePads(MachineFunction &MF) {
----------------
All stuff related to cleanuppad with `__clang_call_terminate` has been removed, here and also other places. It was specially handled before, because in the first proposal, it was the only case with two catches for a single try. The reason why there were two catches for a terminate pad before was explained in this deleted comment block.


================
Comment at: lib/Target/WebAssembly/WebAssemblyUtilities.h:35
-bool isRethrow(const MachineInstr &MI);
-bool isCatch(const MachineInstr &MI);
 bool mayThrow(const MachineInstr &MI);
----------------
I deleted some utility functions because now we have only one `throw`/`catch`/`rethrow` instruction each so it doesn't seem to warrant a separate function like `isCatch`. Also deleted all terminatepad related functions below.


================
Comment at: test/CodeGen/WebAssembly/cfg-stackify-eh.ll:1
 ; RUN: llc < %s -asm-verbose=false -disable-wasm-fallthrough-return-opt -wasm-disable-explicit-locals -wasm-keep-registers -disable-block-placement -verify-machineinstrs -fast-isel=false -machine-sink-split-probability-threshold=0 -cgp-freq-ratio-to-skip-merge=1000 -exception-model=wasm -mattr=+exception-handling | FileCheck %s
 
----------------
I deleted `cfg-stackify-eh.mir` and fixed this test instead because they somehow serve the same purpose. Actually I don't exactly remember why I ended up two different versions.. maybe the mir version was intended for fix unwinding mismatch part (which hasn't landed yet) or something. 


================
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
+
----------------
I deleted `annotations.mir` and created this instead for two reasons:
- Given that we cannot serialize WasmFunctionInfo in mir files, it is not possible to make mir files with registers pass the assembler, because the assembler queries if each register is stackified or not in WasmFuncInfo.
- This is a better way to test the annotation only anyway. I don't remember why I didn't do this in the first place. :$


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57134