Allow pops to validate inside unnamed blocks, removing almost all handleBlockNestedPop() calls by kripken · Pull Request #9292 · WebAssembly/binaryen

kripken · 2026-10-21T23:12:57Z

Uploading a mostly-AI draft to get feedback if we think this is a good direction.
If so I can edit and refine before asking for actual review.

This makes us accept multiple blocks around a pop:

(catch
  (call $foo
    (block (result i32) ;; this is ok, but a name here would not validate
      ..
      (pop i32)
    )
  )
)

This is fine in the binary format, since such blocks vanish. It may look a
little weird in the text format, but I think it is worth allowing this to validate,
as it lets us remove almost all the calls to handleBlockNestedPop():
almost all that our passes do is to emit unnamed blocks. So this removes
a large amount of code, and a constant source of friction when writing new
passes.

I suspect this is easier now than before, since in the past we used to
emit named blocks more often. At some point we made the parser do
more work to avoid that. A nice side effect of that is that it makes this
PR possible.

Thoughts @aheejin @tlively ?

tlively · 2026-10-21T23:18:55Z

I guess this won't help with the fixups necessary in #9289 because those are fixing up pops that have been moved out of leftmost-leaf position. But this direction makes sense to me!

tlively · 2026-10-23T16:19:01Z

Meanwhile my fuzzer found and fixed another nested pop bug in RemoveUnusedBrs. I won't bother uploading the fix since this will make it obsolete soon.

aheejin · 2026-10-25T05:21:03Z

Nice idea! I only skimmed the code but I like this approach.

Will there be cases that some blocks needlessly have names (without targeting branches)? Do we have a pass that removes those unnecessary names, or all the optimization passes that create nested pops ensure they only create unnamed blocks?

kripken · 2026-10-26T16:16:46Z

Will there be case that some blocks needlessly have names (without targeting branches)? Do we have a pass that removes those unnecessary names, or all the optimization passes that create nested pops ensure they only create unnamed blocks?

The very common case is makeSequence which makes an unnamed block. I think Inlining creates named blocks, one of the few to do so (after inlining, a return is a branch to the new block), but RemoveUnusedNames removes those names if they aren't branched to. So we should be pretty efficient here.

kripken · 2026-10-26T18:50:59Z

Ok, fuzzed and manually checked, this is ready for review.

gemini-code-assist

Code Review

This pull request refactors the handling of pop instructions in WebAssembly exception handling. Previously, various optimization passes attempted to manually fix up pop instructions when they were moved into blocks, often by hoisting them into local variables. The new approach simplifies this by allowing pop instructions to reside within unnamed blocks, which is valid in the binary format, and updating the IR builder and validation logic to support this. The changes remove the now-unnecessary EHUtils::handleBlockNestedPops utility and associated state tracking from multiple optimization passes, significantly cleaning up the codebase.

gemini-code-assist · 2026-10-26T18:53:00Z

+      break;
+    }
+    auto* block = scope.getBlock();
+    if (!block || block->name || !scope.exprStack.empty()) {


In Binaryen, Name objects should be checked using the .is() method rather than directly as a boolean, to ensure consistency and avoid potential compilation or implicit conversion issues. Using block->name.is() is also consistent with how it is checked in src/ir/eh-utils.cpp in this same pull request.

if (!block || block->name.is() || !scope.exprStack.empty()) {

Hmm, the current code is consistent with other places in this file, but we don't have consistency in other files, that is true. Separately from this PR, we should consider which way to standardize on.

I definitely tend not to use .is().

tlively

I had Gemini look for stale comments in the tests:

test/lit/passes/code-folding-eh-legacy.wast:376-377 (";; ... so we need a fixup for the pop.")
test/lit/passes/dae2.wast:3432-3433 (";; ... This is invalid, so we must fix it up.")
test/lit/passes/dce-eh-legacy.wast:199-200 (";; ... which needs to be extracted out of the block at the end.")
test/lit/passes/constraint-analysis-eh.wast:63-64 (";; ... This adds a block we need to fix up EH for.")
test/lit/passes/directize_all-features.wast:2048-2049 (";; ... We emit a block with the pop, and must handle that properly.")

And in the code:

src/ir/eh-utils.h:35-38 ("// ... fixes up 'pop's nested in blocks, which are currently not supported without block param types ...") — should qualify that this now only applies to named blocks.
src/ir/eh-utils.cpp:124-130 ("// Change code like ... (catch $e ... (block (pop i32)))") — the example diagram still shows an unnamed block, which is no longer modified by handleBlockNestedPop().
src/wasm/wasm-ir-builder.cpp:52-55 ("// ... generates synthetic Pop Expressions and performs fixups for pops within nested blocks (handleBlockNestedPops).") — fixups are only for named blocks now.
src/wasm/wasm-ir-builder.cpp:164-167 ("// ... help us determine later whether we need to run the nested pop fixup.") — the synthetic block created here is unnamed, so it no longer triggers a nested pop fixup.
src/wasm-ir-builder.h:538-542 ("// ... possible that the pop is inside the synthetic block and we should run the fixup ...") — same as above; synthetic blocks are unnamed and never need fixup now.

It also noticed that we could remove the nested pop fixup in DAE2.

tlively · 2026-10-26T23:07:51Z

It looks like none of these tests actually require a fixup anymore. We should add some named blocks to continue testing the fixup.

Ah, I thought that wasn't possible, but I realize a manual named block achieves the goal... good point, added now.

tlively · 2026-10-26T23:08:45Z

-    EHUtils::handleBlockNestedPops(curr, *getModule());
-  }
-}
+void ReFinalize::visitFunction(Function* curr) {}


Suggested change

void ReFinalize::visitFunction(Function* curr) {}

This is already the default implementation.

Good catch, fixed.

tlively · 2026-10-26T23:21:15Z

-  if (!scope.getCatch() || scope.exprStack.size() != 1 ||
-      !scope.exprStack[0]->is<Pop>()) {
+  // type as the Pop we have already made, and move it into the current scope if
+  // we have entered unnamed blocks since the catch began.


I don't think we have any way of knowing whether a block will be named in advance. Consider this function:

(func $oops try catch $i32 block pop ;; block looks unnamed, so we move the generated pop inside it drop br 0 ;; synthesizes a name for the block end end )

Do we still run the pop fixup later? If so, it's probably fine to move the pops speculatively and let them get fixed up later. We'll end up with a scratch local either way. It might be worth calling that out in the comments, though.

Good point, yes, the block is currently unnamed but may get named later. The function-level fixups that happen afterwards will handle that case (line 1192). I'll add a comment to clarify.

tlively · 2026-10-26T23:27:08Z

(catch
  (call $foo
    (block (result i32) ;; this is ok, but a name here would not validate
      ..
      (pop i32)
    )
  )
)

The .. and (pop i32) here should be transposed, unless the .. is supposed to stand for more unnamed blocks.

kripken · 2026-10-27T17:28:02Z

I had Gemini look for stale comments in the tests:

test/lit/passes/code-folding-eh-legacy.wast:376-377 (";; ... so we need a fixup for the pop.")

Did Gemini run on the wrong branch? That text is updated in this PR:

https://github.com/WebAssembly/binaryen/pull/9292/changes#diff-fd5a5d708a3f62cebeb4135487ff2e5802de88c50120cef6406ea681c307cc87L376-R371

kripken · 2026-10-27T17:31:51Z

The code suggestions look like they are on valid code... but I disagree with them, after reading through them all. I think "fix up pops in blocks" is clear enough, we don't need to add "named" there, that is an internal detail.

Though the example should have a named block, I fixed that.

kripken · 2026-10-27T17:32:39Z

It also noticed that we could remove the nested pop fixup in DAE2.

I don't think that is correct! I undid it here:

2eda71e

The trampolines are named.

tlively · 2026-10-27T17:46:14Z

Did Gemini run on the wrong branch? That text is updated in this PR:

Huh, possibly. Sorry for the noise :(

tlively · 2026-10-27T17:58:48Z

It also noticed that we could remove the nested pop fixup in DAE2.

I don't think that is correct! I undid it here:

2eda71e

The trampolines are named.

The named trampolines are only created to wrap blocks that were also named in the input, so they should never contain pops.

This reverts commit 2eda71e.

kripken · 2026-10-27T20:41:29Z

Sounds good about the trampolines in DAE2, I didn't realize they only happened in that situation. I fixed that.

tlively · 2026-10-27T21:29:45Z

+      break;
+    }
+    auto* block = scope.getBlock();
+    if (!block || block->name || !scope.exprStack.empty()) {


We should check that the block's declared input type is a supertype of the pop's type, too.

Wait, we don't require that, do we? This is valid:

(catch $e-i32 (throw $e-i32 (block (result i32) (pop i32) ;; pop is within an *un*named block, which is valid ) ) )

The block has no input type.

It's valid when parsed into IR, but only because IR doesn't know about block input types. If pop were a real instruction with type [] -> [] (like a nop, which is how we emit it), then this would not be valid WAT. It's probably fine if you don't want to change this here, but I might end up changing it in the PR that optimizes block inputs. Can you add a test case demonstrating the current behavior?

Ah, requiring the block input parameters would make round-tripping through text tricky. We'd have to start printing input parameters on blocks containing pops from Print.cpp. Let's definitely not require this here, then.

tlively · 2026-10-27T21:31:37Z

+  // we have entered unnamed blocks since the catch began. (Note that the block
+  // may receive a name later, if we encounter a break to it, if we process
+  // that after the pop. In that case, the fixups that run later will have work
+  // to do.)


There's a lot of overlap between pushing pops down into unnamed blocks and pushing block input scratch local gets down into unnamed blocks. In both cases, the unnamed blocks must have the proper input types for the code to be valid. In both cases, we're taking a synthetic instruction we've already generated and moving it into a new block.

One difference is that pop can appear in test source code when we round-trip WAT, so in addition to generating it in the right place, we also have to validate its position in the input. It's unclear to me whether we also want to preserve pop positions in the IR. For example, consider this WAT:

try catch $i32 block (param i32) (result i32) ;; A pop block (param i32) (result i32) ;; B end end end

For block input scratch locals, we would always want to move the local.get into the most deeply nested block possible. If we do the same for pop, the parsed pop would be at the beginning block B above, even though the WAT has it at the beginning of block A.

On one hand, parsing pop with its position matching the input gives us more control over how the WAT gets parsed when we write tests.

On the other hand, parsing pop in the innermost valid location is always optimal and matches what we want to do for block input scratch locals. This would also let us share almost all the code for the scratch local placement and pop placement. It wouldn't regress tests that are supposed to directly show the structure of the popped IR because such tests do not have blocks with input parameters to begin with.

If we always parse pop in the innermost valid location, and we eventually share that code with the code for optimizing scratch local placement, then we're still left with makePop being responsible for validation of the input pop. Validating that the input pop is also in the optimal location would be complicated because it would require reading ahead to see if there is a more optimal location after the pop. I think it makes most sense to allow the input to contain a pop as the first child of any unnamed nested block, not just the innermost one.

I don't think this all affects this PR much, but the PR to optimize block input scratch locals will probably replace most of this new code, so I want to make sure we're aligned on direction.

I agree this might get complicated later, but I hoped we can deprecate legacy EH before that... but I guess that did not take into account your plans for block input scratch locals...

I'd be ok changing this later to allow pops in more places, if that is helpful.

tlively · 2026-10-27T21:34:27Z

+ (func $stacky-named-block
+  try
+  catch $e
+   ;; Add a named block here, forcing a fixup using a local to capture the pop.


The scratch local is added when IRBuilder sees the block input parameter, not because the fixup runs.

It would be good to add a test where the block is not named but is targeted by a later branch. That case will definitely need the fixup, at least once we stop creating block input parameters in cases where there is already a pop available.

Thanks, this is tricky as the two fixups look very similar... but in the last commits I think I figured out how to test both.

tlively · 2026-10-27T23:49:07Z

+    pop i32
    nop
    drop
+    br 0


Please add a version of this test without the br as well.

kripken added 3 commits October 20, 2026 16:18

fix?

305995b

fix?

8acacc1

work

5347eaa

kripken mentioned this pull request Oct 21, 2026

[CFP][GSI] Fix catch pops after optimizing reads #9289

Open

kripken added 2 commits October 21, 2026 16:23

Merge remote-tracking branch 'origin/main' into no.eh

5359a9b

handle a pass that just got Pop handling

16e6294

tlively added a commit that referenced this pull request Oct 22, 2026

replace getFirstPop, use PostWalker, depend on #9292

6f9cac0

kripken added 4 commits October 26, 2026 09:18

Merge remote-tracking branch 'origin/main' into no.eh

33ac61b

docs

e889b8d

undo DAE, as it emits trampolines - blocks with new names

2eda71e

update test text

b440d9e

kripken marked this pull request as ready for review October 26, 2026 18:50

kripken requested a review from a team as a code owner October 26, 2026 18:50

kripken requested review from aheejin and stevenfontanella and removed request for a team and stevenfontanella October 26, 2026 18:50

gemini-code-assist Bot reviewed Oct 26, 2026

View reviewed changes

tlively reviewed Oct 26, 2026

View reviewed changes

fix.example

baf5ac8

add a fixup test with a manually-named block, to force a fixup

92d3acf

kripken added 2 commits October 27, 2026 10:35

comment

4a1ccbf

remove unneded visitFunction

b654e78

kripken added 2 commits October 27, 2026 13:38

comment on pop fixup happening later in wasm-ir-builder

8477bc0

Revert "undo DAE, as it emits trampolines - blocks with new names"

62077c9

This reverts commit 2eda71e.

tlively reviewed Oct 27, 2026

View reviewed changes

kripken added 4 commits October 27, 2026 15:43

add explicit spec test for unnamed blocks

d01c2db

make the old test precise and fix comment

af9febd

add a new test for actual fixups

12e2f2f

make the new test really use EH fixups

5fe525d

tlively reviewed Oct 27, 2026

View reviewed changes

add testcase without br

7b3e210

Conversation

kripken commented Oct 21, 2026

Uh oh!

tlively commented Oct 21, 2026

Uh oh!

tlively commented Oct 23, 2026

Uh oh!

aheejin commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kripken commented Oct 26, 2026

Uh oh!

kripken commented Oct 26, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Oct 26, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Oct 26, 2026

Uh oh!

kripken commented Oct 27, 2026

Uh oh!

kripken commented Oct 27, 2026

Uh oh!

kripken commented Oct 27, 2026

Uh oh!

tlively commented Oct 27, 2026

Uh oh!

tlively commented Oct 27, 2026

Uh oh!

kripken commented Oct 27, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

aheejin commented Oct 25, 2026 •

edited

Loading