[all-commits] [llvm/llvm-project] 2bf71b: [WebAssembly] Fix phi handling for Wasm SjLj (#99730)

[Heejin Ahn via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 2bf71b8bc851b49745b795f228037db159005570
      https://github.com/llvm/llvm-project/commit/2bf71b8bc851b49745b795f228037db159005570
  Author: Heejin Ahn <aheejin at gmail.com>
  Date:   2024-07-23 (Tue, 23 Jul 2024)

  Changed paths:
    M llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp
    A llvm/test/CodeGen/WebAssembly/lower-wasm-ehsjlj-phi.ll

  Log Message:
  -----------
  [WebAssembly] Fix phi handling for Wasm SjLj (#99730)

In Wasm SjLj, longjmpable `call`s that in functions that call `setjmp`
are converted into `invoke`s. Those `invoke`s are meant to unwind to
`catch.dispatch.longjmp` to figure out which `setjmp` those `longjmp`
buffers belong to:

https://github.com/llvm/llvm-project/blob/fada9227325b3eaa0bdc09a486f29a7f08b7b3fb/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp#L250-L260

But in case a longjmpable call is within another `catchpad` or
`cleanuppad` scope, to maintain the nested scope structure, we should
make them unwind to the scope's next unwind destination and not directly
to `catch.dispatch.longjmp`:

https://github.com/llvm/llvm-project/blob/fada9227325b3eaa0bdc09a486f29a7f08b7b3fb/llvm/lib/Target/WebAssembly/WebAssemblyLowerEmscriptenEHSjLj.cpp#L1698-L1727
In this case the longjmps will eventually unwind to
`catch.dispatch.longjmp` and be handled there.

In this case, it is possible that the unwind destination (which is an
existing `catchpad` or `cleanuppad`) may already have `phi`s. And
because the unwind destinations get new predecessors because of the
newly created `invoke`s, those `phi`s need to have new entries for those
new predecessors.

This adds new preds as new incoming blocks to those `phi`s, and we use a
separate `SSAUpdater` to calculate the correct incoming values to those
blocks.

I have assumed `SSAUpdaterBulk` used in `rebuildSSA` would take care of
these things, but apparently it doesn't. It takes available defs and
adds `phi`s in the defs' dominance frontiers, i.e., where each def's
dominance ends, and rewrites other uses based on the newly added `phi`s.
But it doesn't add entries to existing `phi`s, and the case in this bug
may not even involve dominance frontiers; this bug is simply about
existing `phis`s that have gained new preds need new entries for them.
It is kind of surprising that this bug was only reported recently, given
that this pass has not been changed much in years.

Fixes #97496 and fixes
https://github.com/emscripten-core/emscripten/issues/22170.



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications