[tlively]

When entering a nested control flow scope whose input parameters are scratch local.gets from the enclosing scope's inputs, reuse the enclosing scope's scratch locals directly instead of allocating new scratch locals. Share the movement logic in pushScope with moving legacy EH pops into nested unnamed blocks.

[gemini-code-assist]

Code Review

This pull request optimizes input parameter handling in IRBuilder::pushScope by allowing pops to move into unnamed blocks and reusing scratch locals from enclosing scopes instead of allocating new ones. It also simplifies pop validation in IRBuilder::makePop and adds comprehensive tests for block tunneling and misplaced pop parse errors. The review feedback points out a critical correctness bug in pushScope where isInputGet fails to verify that the LocalGet index matches the parent local's index, which could trigger an assertion failure or cause incorrect optimization. Additionally, the reviewer suggests wrapping the parameter matching loop in an if (hasInput) block to prevent potential out-of-bounds indexing of Type::none when hasInput is false.

[github-actions]

🤖 Google Antigravity Code Review

I have reviewed the changes in this Pull Request and left inline feedback.

[aheejin]

It looks this also contains changes in #9336. Are you going to rebase?

[tlively]

Yes, merged latest changes now.

[tlively]

Friendly ping @aheejin

[aheejin]

1. Can we split the pop handling code and the scratch local tunneling code? They don't share much other than pushSynthetic at the end, and to me merging the two code paths makes it harder to read.

2. Currently this code is accepted without an error:

(module
  (tag $e (param i64))
  (func $test
    try
    catch $e
      block (param i32)  ;; This should error out because the tag type is i64
        drop
      end
    end
  )
)

3. Can we have a test case that tests double tunneling too, so that we can confirm we only create a single scratch local for the below?

(func $block-double-tunnel (result i32)
  i32.const 0
  block (param i32) (result i32)
    block (param i32) (result i32)
      block (param i32) (result i32)
      end                                                                                                                                                                                                                         
    end                                                                                                                                                                                                                           
  end                                                                                                                                                                                                                             
)    

[tlively]

1. Can we split the pop handling code and the scratch local tunneling code? They don't share much other than pushSynthetic at the end, and to me merging the two code paths makes it harder to read.

Yes, this makes things much simpler. Good idea.

3. Currently this code is accepted without an error:
   (module
   (tag $e (param i64))
   (func $test
   try
   catch $e
   block (param i32) ;; This should error out because the tag type is i64
   drop
   end
   end
   )
   )

Good catch, fixed.

3. Can we have a test case that tests double tunneling too, so that we can confirm we only create a single scratch local for the below?

(func $block-double-tunnel (result i32)
i32.const 0
block (param i32) (result i32)
block (param i32) (result i32)
block (param i32) (result i32)
end
end
end
)

Done, and added some other interesting test cases as well.

[aheejin]

In the PR description

Share the movement logic in pushScope with moving legacy EH pops into nested unnamed blocks.

doesn't seem to be true anymore given that now we separated the paths.

[aheejin]

so we can round-trip WAT without the printer needing to synthesize block input types

What does this mean? Was there any case the printer needed to synthesize block input types? Were we doing this before?

[tlively]

My initial idea was to require unnamed blocks surrounding pops to have parameters like this:

(catch $i32
  (block (param i32)
    (pop i32)
  )
)

The problem with requiring this is that the Printer would then have to print the (param i32) on the unnamed block so the printed WAT could be parsed. We don't want that extra complexity in the printer, and we do want to be able to round-trip through WAT, so we need to allow pops without the params on the surrounding unnamed blocks.

[aheejin]

(catch $i32
  (block (param i32)
    (pop i32)
  )
)

It would be helpful to have this example in the comment.

[tlively]

Done.

[aheejin]

I don't understand what this sentence means. Can you elaborate or give an example?

[tlively]

See $block-tunnel-subtype-mixed in control-flow-input.wast. I've tried to clarify the comment.

[aheejin]

Do we disallow subtypes? We were allowing them, no?

[tlively]

Yes, we were allowing them previously. We can continue allowing them now that pop has a separate control flow path, so I've restored the previous behavior.

[aheejin]

Why was this deleted? I guess this is related to disallowing subtypes in makePop, but I'm not sure why we disallow them. And if we have a reason to disallow, I think we should note this behavior change in the PR description. Or even better, a separate PR.

[tlively]

Yep, restored.

[aheejin]

The CHECK error messages for these three cases are misleading because, in the first case, the real reason is the type mismatch (i32 != i64), and the second and the third cases involve scratch locals that users aren't aware of.

I get why the messages are printed this way, and I'm not suggesting to fix them here, but just noting them. (I'm also not sure if these are worth fixing for the sake of error messages, if fixing these involves some tricky code restructuring. Anyway, we shouldn't fix them here in this PR)

[tlively]

Yep, agreed. I've added TODOs about better error messages.

[aheejin]

Maybe it's only me, but I still wouldn't have understood this comment without you pointing to the example. I think one reason I feel this is confusing is, this comment is describing a specific case (like $block-tunnel-subtype-mixed) where earlier inputs were widened and thus using new scratch locals and the later locals were reused, but this sounds like that's what the code below is doing in general. But the code below works in all general cases, including when earlier inputs are reused and instead later inputs get new scratch locals, etc. So it would've been helpful if this sentence had started like "Even in the case earlier inputs that ~"

[tlively]

Makes sense. Clarified further.

[aheejin]

LGTM % comment suggestions

[github-actions]

🤖 Google Antigravity Code Review

I have reviewed the changes in this Pull Request and left inline feedback.

[github-actions]

[CRITICAL | Confidence: 5/5 | BUG]

Unconditionally tunneling the Pop into an unnamed block when scope.inputType == Type::none assumes that any unnamed block at the beginning of a catch is meant to consume or contain the Pop. In WebAssembly, a block with Type::none input ([] -> [...]) does not pop any value from the value stack; values on the stack prior to entering the block remain on the outer stack and are preserved across the block.

If a catch block begins with an unnamed block that does not contain a pop instruction (such as an unnamed block performing computation or side-effects before the exception payload is used or dropped, e.g., (catch $e (block (nop)) (drop))):

1. pushScope removes Pop from catchScope.exprStack and pushes it into the unnamed block's exprStack.
2. When the block ends, Pop is included in Block::list as an unused, un-dropped non-final expression, violating Binaryen IR invariants ("non-final block elements returning a value must be dropped") and failing validation.
3. The enclosing catch scope loses Pop from its stack, causing subsequent operations in the catch scope that consume the payload (e.g., drop or arithmetic) to fail with a parse or type error (e.g. popping from an empty stack).

Moving the Pop eagerly when scope.inputType == Type::none is premature because pushScope runs before any inner instructions are visited. Tunneling in pushScope should only occur when the block explicitly declares input parameters matching the pop (scope.inputType != Type::none), whereas handling (pop ...) within blocks that have no input parameters should be handled lazily when makePop is called.

[tlively]

Hmm, this is a good point actually. This perfectly valid wat fails to parse:

(module
  (tag $i32 (param i32))
  (func (result i32)
    (try (result i32)
      (do
        (i32.const 0)
      )
      (catch $i32
        (block ;; synthetic pop moves into this block
          (nop)
        )
        ;; but it should have stayed in the outer scope
        ;; and been hoisted to here with a scratch local
      )
    )
  )
)

I guess I should fix this before landing 😢

[aheejin]

Can we fix this without just removing this pop part in this PR and restoring what #9292 did, meaning, can we tunnel pops unconditionally in pushScope without hitting this bug?

If not we can just remove the pop handling part in this PR. Even if we end up with something else with pop, I think it's still a good idea to extract the pop thing out of this PR, because, at this point, tunneling scratch locals is just separate thing from tunneling pops. Whichever way, we can land this now without the pop thing.

[tlively]

I was going to fix it by requiring the unnamed blocks to have input parameters and fixing the Printer to emit those input parameters as well, but you're right that we can also just leave the pop handling the way it was before this PR. I'll do that.