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

[Heejin Ahn via Phabricator via llvm-commits]

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

Thanks for the review, and sorry for delayed reply.



================
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;
----------------
tlively wrote:
> 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?
Yes. We do this for the normal `block` or `try` placement too.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:911-912
+    //   range_end
+    // delegate_bb: (new)
+    //   delegate
+    // post_bb: (new)
----------------
tlively wrote:
> The delegate needs to be in its own new BB because it is both a terminator and a destination, right?
Yes. The same reason with that `catch` starts a new BB.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:921
+    PostBB->transferSuccessors(PreBB);
+    unstackifyVRegsUsedInSplitBB(*PreBB, *PostBB);
+    PreBB->addSuccessor(DelegateBB);
----------------
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.


================
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))
----------------
tlively wrote:
> 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?
Yes.


================
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;
----------------
tlively wrote:
> Are the EH_LABELS a kind of metadata node? They can only be located immediately before or after the relevant MachineInstr?
AFAIK they are placed in two cases:
- Before and after a call that was previously an invoke
```
EH_LABEL ...
CALL @foo ...
EH_LABEL
```
- In the beginning of an EH pad
```
EH_LABEL ...
CATCH ...
...
```

We don't use EH labels for anything for now, but try to preserve the relationship between EH label instruction(s) and their relevant instruction if possible to follow the convention. In this case, calls (previously invokes) are wrapped with `EH_LABEL` and we don't want to place `try` or `delegate` between these EH label instructions and the invoke.


================
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;
----------------
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.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyCFGStackify.cpp:1212
+
+  return true;
+}
----------------
tlively wrote:
> Would it be worth being more precise here? IIUC, this could return false if there were no mismatches.
We do early exit on that case. See line 1178:
```
  // We don't have any unwind destination mismatches to resolve.
  if (UnwindDestToTryRanges.empty())
    return false;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94048