[PATCH] D94048: [WebAssembly] Fix call unwind mismatches

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

Looks good overall! Most of these comments are just me checking my understanding.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:865-866
+
+  // Local expression tree before the first call of this range should go
+  // after the nested TRY.
+  SmallPtrSet<const MachineInstr *, 4> AfterSet;
----------------
Is this to ensure that the arguments to the call are inside the `try` so that the `try` itself does not need to take any stack arguments?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:888
+  MachineBasicBlock *DelegateBB = MF.CreateMachineBasicBlock();
+  // If the destination of 'delegate' is not the caller, adds the destnation to
+  // the BB's successors.
----------------



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:911-912
+    //   range_end
+    // delegate_bb: (new)
+    //   delegate
+    // post_bb: (new)
----------------
The delegate needs to be in its own new BB because it is both a terminator and a destination, right?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:921
+    PostBB->transferSuccessors(PreBB);
+    unstackifyVRegsUsedInSplitBB(*PreBB, *PostBB);
+    PreBB->addSuccessor(DelegateBB);
----------------
It seems like it might be simpler to do these CFG transformations before register stackification. Is there a reason we do them after register stackification?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:940
+  // only 'catch' but all block-like structures including another 'delegate',
+  // but with a slightly different semantics with branches. When it targets a
+  // 'catch', it will delegate the exception to that catch. It is being
----------------



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:1076
+      // successor or MI does not throw, this is not an invoke.
+      if (SeenThrowableInstInBB || !MBB.hasEHPadSuccessor() ||
+          !WebAssembly::mayThrow(MI))
----------------
Can this `!MBB.hasEHPadSuccessor()` be checked outside this inner loop over the instructions? I guess not because the `EHPadStack` still needs to be updated, right?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:1098
+
+      // Include EH_LABELs in the range before and afer the invoke
+      MachineInstr *RangeBegin = &MI, *RangeEnd = &MI;
----------------
Are the EH_LABELS a kind of metadata node? They can only be located immediately before or after the relevant MachineInstr?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:1147-1149
+      // If MBB has an EH pad successor, this inst does not unwind to caller.
+      if (MBB.hasEHPadSuccessor())
+        continue;
----------------
Can there be a BB with a call that throws to the caller followed by an invoke that unwinds to an EH pad? In that case wouldn't the BB have an EH pad successor, but still contain an unwind to the caller?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:1212
+
+  return true;
+}
----------------
Would it be worth being more precise here? IIUC, this could return false if there were no mismatches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94048