fix: resolve function inlining idempotence issue#40
Closed
Conversation
## Root Cause The idempotence test was failing because of a bug in how custom sections (specifically the "name" section) were being encoded during WASM round-trips: 1. When parsing WASM, we used `reader.range()` to extract custom section bytes 2. The range included the section payload WITH the name field 3. When re-encoding, wasm-encoder added the name field again 4. Result: Each encode/parse cycle duplicated the name field, growing the file This caused parse → encode → parse → encode to produce different sizes (89 bytes → 94 bytes), making the idempotence test fail. ## Fix 1. Changed custom section parsing to use `reader.data()` instead of `reader.range()` - This extracts only the section data WITHOUT the name field - The encoder properly adds the name field during re-encoding - Now encode/parse cycles are deterministic 2. Made inline_functions run to fixed point - Loops until no more inlining candidates found - Ensures true idempotence even if new opportunities arise - More robust optimization behavior 3. Re-enabled the idempotence test - Removed #[ignore] attribute - Test now passes consistently ## Testing - All existing tests pass - Idempotence test now passes - WASM encoding is deterministic across multiple round-trips Note: advanced_math.wat and crypto_utils.wat still fail validation due to a separate stack discipline bug in the inlining logic itself (not related to idempotence). These remain skipped in CI per the existing workaround.
## Problem The `eliminate_redundant_sets` optimization in `simplify-locals` was incorrectly removing stack-producing instructions, causing invalid WASM output with "values remaining on stack" errors. ## Root Cause The optimization assumed that the value for a `local.set` was always produced by the immediately preceding instruction (`result.len() - 1`). This is incorrect in stack-based WASM where values can come from complex expression trees spanning multiple instructions. Example of buggy behavior: ```wat Input: local.get $base i32.const 3 i32.shl # ← Produces value for local.set local.set $temp1 Output (broken): local.get $base i32.const 3 # i32.shl REMOVED! ← Bug: incorrectly removed local.set $temp1 ``` ## Fix Disabled the `eliminate_redundant_sets` optimization by commenting it out in `simplify_locals` (loom-core/src/lib.rs:4155). The optimization needs to be rewritten to properly understand the stack discipline and track which instructions actually produce values for sets. ## Changes 1. **loom-core/src/lib.rs:4155** - Commented out eliminate_redundant_sets call 2. **loom-core/src/lib.rs:4369** - Added `#[allow(dead_code)]` to function 3. **loom-core/tests/optimization_tests.rs** - Marked RSE tests as `#[ignore]` 4. **.github/workflows/ci.yml** - Removed skips for advanced_math.wat and crypto_utils.wat ## Testing - ✅ Both advanced_math.wat and crypto_utils.wat now validate correctly - ✅ All other tests pass - ✅ Idempotence test still passes ## Impact The `simplify-locals` pass still performs other optimizations (local variable equivalence, redundant get/set elimination). Only the buggy redundant set elimination is disabled.
Contributor
Author
|
Closing as obsolete - the fixed-point loop for inline_functions is already in main, and the idempotence test passes. The eliminate_redundant_sets bug may have been fixed separately or doesn't trigger anymore. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #33
Summary
inline_functionsrun to fixed point for true idempotenceRoot Cause
The idempotence test was failing because custom sections (specifically the "name" section) were duplicating the name field on each encode/parse cycle:
reader.range()which included the name field in the payloadCustomSection { name, data }Changes
reader.data()instead ofreader.range()for custom sections#[ignore]from idempotence testTesting
Note
advanced_math.watandcrypto_utils.watstill fail due to a separate stack discipline bug (not idempotence related). They remain skipped in CI.