[tlively]

Input scratch locals are never modified while their initial local.get expressions remain on the expression stack, so hoistLastValue can move them directly across subsequent none-typed expressions without allocating a new scratch local or creating a synthetic block. Track whether a local is an input local of the current scope via ScopeCtx::isInputLocal and use the same check when tunneling inputs in pushScope.

Note that optimizing input parameters in IRBuilder cannot disturb tests that expect the parsed IR to closely mirror the input WAT, because Binaryen IR does not have control flow input parameters and such tests therefore do not use them.

[gemini-code-assist]

Code Review

This pull request optimizes WebAssembly IR building by hoisting input scratch locals directly across none-typed expressions, avoiding unnecessary scratch local allocations. It also simplifies the scope-pushing logic by checking if a local is an input local directly. The review feedback suggests replacing std::find in the header with a range-based loop to avoid an <algorithm> dependency, and guarding against potential out-of-bounds indexing on scope.inputType when processing inputs.

[gemini-code-assist]

Using std::find in the header file introduces a dependency on <algorithm>. Since inputLocals is typically a very small vector, we can replace it with a simple range-based for loop to avoid this dependency and keep the header self-contained.

    bool isInputLocal(Index local) const {
      for (auto inputLocal : inputLocals) {
        if (inputLocal == local) {
          return true;
        }
      }
      return false;
    }

[gemini-code-assist]

When hasInput is false, scope.inputType is Type::none (which has a size of 0). If inputs is populated (e.g., in the isUnnamedBlock and Pop case), accessing scope.inputType[i] results in an out-of-bounds indexing on Type. Although this is currently short-circuited because get is nullptr for Pop, it is extremely fragile. We should safely guard the indexing by checking the bounds of scope.inputType.

      auto* get = expr->dynCast<LocalGet>();
      Type type = i < scope.inputType.size() ? scope.inputType[i] : Type::none;
      bool isInputGet = get && get->type == type &&
                        getScope().isInputLocal(get->index);
      bool isPop = isUnnamedBlock && inputs.size() == 1 && expr->is<Pop>();
      if (isInputGet) {
        scope.inputLocals.push_back(get->index);
      } else if (!isPop) {
        auto scratch = addScratchLocal(type);
        CHECK_ERR(scratch);

[kripken]

1. Perhaps it makes sense to document the invariant depended on here in wasm-ir-builder.h?
2. This invariant seems right to me, but I asked Gemini to verify, and it came up with... this:

(module
  (func (export "test") (result i32)
    i32.const 10
    block (param i32) (result i32)
      i32.const 99
      local.set 0 ;; (or `local.set $scratch` in WAT) — accepted & passes validation!
    end
  )
)

$ bin/wasm-opt a.wat --print
(module
 (type $0 (func (result i32)))
 (export "test" (func $0))
 (func $0 (result i32)
  (local $scratch i32)
  (local $scratch_1 i32)
  (local.set $scratch
   (i32.const 10)
  )
  (block (result i32)
   (local.set $scratch_1
    (local.get $scratch)
   )
   (local.set $scratch
    (i32.const 99)
   )
   (local.get $scratch_1)
  )
 )
)

This should not even validate (seems like a bug) but it manages to write to a scratch local in an unexpected way. Not to the live local, but still, this looks concerning...

[tlively]

Oh yeah, we shouldn't allow accessing scratch locals (introduced for any reason) by index like that. But fixing that mostly-unrelated bug will restore this invariant.

1. Perhaps it makes sense to document the invariant depended on here in wasm-ir-builder.h?

This is all internal implementation details of the control flow input lowering, so I'm not sure why the header would be the right location for a comment.

[kripken]

Fair enough about a comment. Otherwise lgtm, but I think we need to fix that bug before this lands, as the fuzzer should be able to error until then.

[tlively]

I don't think the fuzzer would be affected. It should never be parsing modules that incorrectly access scratch locals via out-of-bounds local indices.

[kripken]

Hmm, I guess it would take a text-level fuzzer, yeah, good point. Needs to be an invalid module.