[kripken]

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]

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]

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]

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]

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]

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]

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()) {

[kripken]

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.

[tlively]

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]

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

[tlively]

Suggested change

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

This is already the default implementation.

[tlively]

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.

[tlively]

(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.