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

[Thomas Lively via Phabricator via llvm-commits]

tlively accepted this revision.
tlively added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:921
+    PostBB->transferSuccessors(PreBB);
+    unstackifyVRegsUsedInSplitBB(*PreBB, *PostBB);
+    PreBB->addSuccessor(DelegateBB);
----------------
aheejin wrote:
> tlively wrote:
> > 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?
> These transformations can happen only after CFGStackify, which has to follow CFGSort. And RegColoring has to be after RegStackify. So if we want to do this fixing unwind mismatches before RegStackify, the pass order would be like:
> CFGSort -> CFGStackify -> RegStackify -> RegColoring -> ExplicitLocals
> 
> This might be possible, but I haven't experimented with this. One thing that comes to mind is if we place `block`/`try` marker first and do RegStackify afterwards we may hamper some register stackification opportunities due to those markers, but this also might be solvable with care.
> 
> So the short answer is we might be able to, but we have stayed with this order for a long time and I'm not very sure about all the consequences if we change this. I think the original intention of the pipeline is we don't make any CFG-level changes after FixIrreducibleControlFlow. Unfortunately this fixing unwind mismatch transformation broke that assumption, which has to be after CFGStackify marker placement.
Got it, thanks for the detail!


================
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;
----------------
aheejin wrote:
> tlively wrote:
> > 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?
> Good catch; I think you are right, there can be a BB like
> ```
> call $foo ;; unwind to the caller
> call $bar ;; unwind to an EH pad (previously invoke)
> ```
> 
> I changed the code so that we only skip when `MBB.isEHPadSuccessor()` and the instruction is the last throwing instruction. PTAL.
> 
> I think we eventually need to devise a way to carry IR's `nounwind` attribute to our backend in order to prevent treating too many non-throwing calls to possibly-throwing-to-caller ones.
Would it be worthwhile to add a test for this case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94048