Fix incorrect mstore removal. - #16459
Open
rodiazet wants to merge 6 commits into
Open
Conversation
rodiazet
force-pushed
the
fix-mstore-removal
branch
2 times, most recently
from
February 11, 2026 10:56
c987b2b to
38fd9ac
Compare
rodiazet
force-pushed
the
fix-mstore-removal
branch
3 times, most recently
from
February 19, 2026 15:58
16b2cc9 to
30c820c
Compare
|
There was an error when running Please check that your changes are working as intended. |
rodiazet
force-pushed
the
fix-mstore-removal
branch
4 times, most recently
from
February 19, 2026 17:15
cba7f5b to
31b25f9
Compare
rodiazet
marked this pull request as ready for review
February 19, 2026 17:15
There was a problem hiding this comment.
Pull request overview
This PR updates the Yul optimizer’s UnusedStoreEliminator pipeline to avoid incorrect mstore removals when the input is not in pseudo-SSA form (notably when function parameters are involved), and adds targeted optimizer tests for the “no SSA transform” scenario described in #16458.
Changes:
- Extend
SSAValueTrackerto treat unassigned function parameters as valid SSA roots when checking SSA-ness transitively through dependencies. - Filter the knowledge-base SSA input used by
UnusedStoreEliminatorto only include expressions that are in SSA form together with their dependencies. - Add a dedicated
unusedStoreEliminatorNoSsaTransformtest step and new Yul tests that exercise the problematic cases.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
libyul/optimiser/SSAValueTracker.h |
Adds an SSA-with-dependencies query API and tracks function parameters. |
libyul/optimiser/SSAValueTracker.cpp |
Implements dependency-based SSA checking and parameter tracking during AST walk. |
libyul/optimiser/UnusedStoreEliminator.cpp |
Builds the SSA value map using only SSA-safe expressions (with SSA-safe dependencies). |
test/libyul/YulOptimizerTestCommon.cpp |
Adds a new optimizer test step that runs UnusedStoreEliminator without SSATransform. |
test/libyul/yulOptimizerTests/unusedStoreEliminatorNoSsaTransform/mstore_location_reassigned.yul |
New regression test covering non-SSA reassignment affecting mstore location. |
test/libyul/yulOptimizerTests/unusedStoreEliminatorNoSsaTransform/function_parameter_as_mstore_location_unrelated.yul |
New test where parameter-derived store location should be removable (non-overlapping). |
test/libyul/yulOptimizerTests/unusedStoreEliminatorNoSsaTransform/function_parameter_as_mstore_location_covering.yul |
New test where parameter-derived store location must be kept (overlapping return). |
test/libyul/yulOptimizerTests/unusedStoreEliminatorNoSsaTransform/function_parameter_reassinged_as_mstore_location_unrelated.yul |
New test intended to cover reassigned-parameter non-overlap scenario. |
test/libyul/yulOptimizerTests/unusedStoreEliminatorNoSsaTransform/function_parameter_reassinged_as_mstore_location_covering.yul |
New test covering reassigned-parameter overlapping scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
rodiazet
force-pushed
the
fix-mstore-removal
branch
2 times, most recently
from
February 19, 2026 18:26
9cf23ac to
7d014ec
Compare
Comment on lines
+87
to
+91
| else | ||
| { | ||
| solAssert(std::holds_alternative<Literal>(*_expression), "Impossible expression type"); | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| else | |
| { | |
| solAssert(std::holds_alternative<Literal>(*_expression), "Impossible expression type"); | |
| return true; | |
| } | |
| solAssert(std::holds_alternative<Literal>(*_expression), "Impossible expression type"); | |
| return true; |
Member
There was a problem hiding this comment.
i don't think the suggestion works
| // let a := add(arg, 1) | ||
| // let b := arg | ||
| // let outLen := 32 | ||
| // mstore(a, 0xAA) |
Contributor
There was a problem hiding this comment.
This worked before the fix as well, since you're not reassigning arg?
Contributor
Author
There was a problem hiding this comment.
Yes, but I added function parameters set to the SSAValueTracker, because I need to know them if they are not reassigned and if not they properly close the closed set. This is just a simple test of this.
rodiazet
force-pushed
the
fix-mstore-removal
branch
2 times, most recently
from
February 20, 2026 12:44
3ef065c to
df081a5
Compare
rodiazet
force-pushed
the
fix-mstore-removal
branch
from
February 20, 2026 12:55
bbdfc8c to
b871cd5
Compare
cameel
reviewed
Feb 20, 2026
Collaborator
There was a problem hiding this comment.
Please remember that this will need a buglist entry.
Without it the PR is not really finished so it should still be a draft.
Contributor
Author
There was a problem hiding this comment.
bug list updated. How about the blogpost link? I changed it to draft for now.
Collaborator
There was a problem hiding this comment.
You can leave it unfilled until we know when we're releasing this. URLs are predictable, based on date and title (which should match the bug ID) so we could create one already, but if we do it now, we can easily forget to update the date and we'll end up with a broken link. So for now I'd put in something that clearly looks like a placeholder.
Contributor
Author
There was a problem hiding this comment.
I used X/Y/Z placeholder for now as it was done in prev cases.
rodiazet
force-pushed
the
fix-mstore-removal
branch
from
February 23, 2026 11:50
b871cd5 to
8f69008
Compare
rodiazet
marked this pull request as draft
February 23, 2026 11:51
rodiazet
force-pushed
the
fix-mstore-removal
branch
from
February 23, 2026 13:08
8f69008 to
87e9ec9
Compare
clonker
reviewed
Aug 11, 2026
| @@ -1,4 +1,14 @@ | |||
| [ | |||
| { | |||
| "uid": "SOL-2026-2", | |||
Member
There was a problem hiding this comment.
this needs to be updated before we merge
rodiazet
force-pushed
the
fix-mstore-removal
branch
3 times, most recently
from
August 12, 2026 09:47
14866f5 to
9305aa1
Compare
blishko
reviewed
Aug 12, 2026
blishko
left a comment
blishko
left a comment
Contributor
There was a problem hiding this comment.
Small comments and questions.
Also I did not understand from the discussion if there is an agreement on whether or not this will have a bug list entry.
| { | ||
| "uid": "SOL-2026-4", | ||
| "name": "UnusedStoreEliminatorWithoutSSATransformation", | ||
| "summary": "When the UnusedStoreEliminators optimizer step is run on non-SSA AST form, it may result in incorrect removal of storage or memory writes.", |
Contributor
There was a problem hiding this comment.
Suggested change
| "summary": "When the UnusedStoreEliminators optimizer step is run on non-SSA AST form, it may result in incorrect removal of storage or memory writes.", | |
| "summary": "When the UnusedStoreEliminator optimizer step is run on non-SSA AST form, it may result in incorrect removal of storage or memory writes.", |
| /// YulName does not need to be reset because SSAValueTracker is short-lived. | ||
| Expression const m_zero{Literal{{}, LiteralKind::Number, LiteralValue(u256{0})}}; | ||
| std::map<YulName, Expression const*> m_values; | ||
| std::unordered_map<YulName, Expression const*> m_values; |
Contributor
There was a problem hiding this comment.
Why are you changing ordered std::map to std::unordered_map?
Contributor
There was a problem hiding this comment.
Since this is not related to the fix, maybe we should not bundle it together?
Member
There was a problem hiding this comment.
Yeah we can pull it out. It's a very small self-contained change.
Comment on lines
+75
to
+78
| for (auto const& argument: _call.arguments) | ||
| if (!isSSAWithDependencies(&argument)) | ||
| return false; | ||
| return true; |
Contributor
There was a problem hiding this comment.
Can we replace this with std::all_of?
clonker
reviewed
Aug 12, 2026
clonker
left a comment
clonker
left a comment
Member
There was a problem hiding this comment.
Please also look at LiCM and check what the effects of this fix are. It would be good to add at least one or two LiCM tests with a parameter-referencing invariant, I think.
| * Yul: Fix incorrect serialization of Yul object names containing double quotes and escape sequences, producing output that could not be parsed as valid Yul. | ||
| * Yul EVM Code Transform: Improve stack shuffler performance by fixing a BFS deduplication issue. | ||
| * Yul IR Code Generation: Preserve custom error argument of `require` when stripping of revert strings is selected via `--revert-strings strip`. | ||
| * Yul Optimizer: Fix a bug in `UnusedStoreEliminator`, which could lead to incorrect removal of `mstore` or `sstore` in certain cases. |
Member
There was a problem hiding this comment.
this needs to be moved into the unreleased section
rodiazet
force-pushed
the
fix-mstore-removal
branch
from
August 13, 2026 11:22
9305aa1 to
ece9999
Compare
rodiazet
force-pushed
the
fix-mstore-removal
branch
from
August 13, 2026 13:32
ece9999 to
2110488
Compare
Contributor
Author
Benchmark Comparison: develop vs mstoreBaseline: bench-results-develop.json (0.8.37-develop.2026.8.13+commit.ff3c7124.Darwin.appleclang, n=3) Sorted by winner (BASELINE, TARGET, tie, ~noise), then by |Δ%| descending within each group.
|
Contributor
Can you make the table collapsible? https://gist.github.com/pierrejoubert73/902cc94d79424356a8d20be2b382e1ab |
Contributor
Author
|
I ran for 10 iterations and prepare the table you like. Moreover the Benchmark Comparison: comparision.txtBaseline: 0.8.37-develop.2026.8.14+commit.ff3c7124.Darwin.appleclang (n=10) Original (unsorted) order from solarray-a547630 · evmasm — wall_time -6.72% (TARGET)
solarray-a547630 · ir — wall_time -6.49% (TARGET)
solidity-verifier · evmasm — wall_time -5.65% (TARGET)
solidity-verifier · ir — wall_time -5.86% (TARGET)
solidity-optimizor-club · ir — wall_time -5.62% (TARGET)
solidity-chains · evmasm — wall_time -5.74% (TARGET)
safe-smart-account-1.5.0 · evmasm — wall_time -5.13% (TARGET)
safe-smart-account-1.5.0 · ir — wall_time -5.53% (TARGET)
account-abstraction-0.8.0 · evmasm — wall_time -2.67% (TARGET)
account-abstraction-0.8.0 · ir — wall_time -5.62% (TARGET)
weth9 · evmasm — wall_time -4.4% (TARGET)
weth9 · ir — wall_time -5.55% (TARGET)
oeth · evmasm — wall_time -4.09% (TARGET)
oeth · ir — wall_time -3.91% (TARGET)
aave-v4-hub · evmasm — wall_time -0.98% (TARGET)
aave-v4-hub · ir — wall_time -2.38% (TARGET)
uniswap-v3-pool · evmasm — wall_time -1.78% (TARGET)
uniswap-v3-pool · ir — wall_time -1.72% (TARGET)
chainlink-link-token · evmasm — wall_time -4.24% (TARGET)
chainlink-link-token · ir — wall_time -4.65% (TARGET)
chainlink-ocr2-aggregator · evmasm — wall_time -3.21% (TARGET)
chainlink-ocr2-aggregator · ir — wall_time -3.49% (TARGET)
balancer-v2-vault · evmasm — wall_time -2.4% (TARGET)
balancer-v2-vault · ir — wall_time -3.09% (TARGET)
balancer-v3-router · evmasm — wall_time -1.28% (TARGET)
balancer-v3-router · ir — wall_time -3.39% (TARGET)
balancer-v3-vault · evmasm — wall_time -1.09% (TARGET)
balancer-v3-vault · ir — wall_time -1.39% (TARGET)
boredapeyachtclub · evmasm — wall_time -0.71% (TARGET)
boredapeyachtclub · ir — wall_time -1.12% (TARGET)
cow-protocol-settlement · evmasm — wall_time -0.32% (TARGET)
cow-protocol-settlement · ir — wall_time -2.01% (TARGET)
compound-comet-rewards · evmasm — wall_time -1.41% (TARGET)
compound-comet-rewards · ir — wall_time -1.53% (TARGET)
compound-comet-extended-asset-list · evmasm — wall_time -1.15% (TARGET)
compound-comet-extended-asset-list · ir — wall_time -1.41% (TARGET)
ens-eth-registrar-controller · evmasm — wall_time -0.95% (TARGET)
ens-eth-registrar-controller · ir — wall_time -2.35% (TARGET)
ens-name-wrapper · evmasm — wall_time -1.23% (TARGET)
ens-name-wrapper · ir — wall_time -1.71% (TARGET)
ens-registry-with-fallback · evmasm — wall_time -0.9% (~noise)
ens-registry-with-fallback · ir — wall_time -1.1% (TARGET)
eigenlayer-eigenpod-manager · evmasm — wall_time -0.9% (TARGET)
eigenlayer-eigenpod-manager · ir — wall_time -1.21% (TARGET)
eigenlayer-strategy-manager · evmasm — wall_time -0.92% (TARGET)
eigenlayer-strategy-manager · ir — wall_time -0.24% (~noise)
etherfi-liquidity-pool · evmasm — wall_time -0.88% (~noise)
etherfi-liquidity-pool · ir — wall_time -3.49% (TARGET)
eulerswap · evmasm — wall_time -2.36% (TARGET)
eulerswap · ir — wall_time -2.32% (TARGET)
usdc-fiattoken-v2-2 · evmasm — wall_time -2.32% (TARGET)
usdc-fiattoken-v2-2 · ir — wall_time -3.58% (TARGET)
frax-stablecoin · evmasm — wall_time +1.58% (~noise)
frax-stablecoin · ir — wall_time -2.86% (TARGET)
lido-accounting-oracle · evmasm — wall_time -2.83% (TARGET)
lido-accounting-oracle · ir — wall_time -3.69% (TARGET)
lido-withdrawal-queue · evmasm — wall_time -2.95% (TARGET)
lido-withdrawal-queue · ir — wall_time -3.83% (TARGET)
lido-wsteth · evmasm — wall_time -3.79% (TARGET)
lido-wsteth · ir — wall_time -3.8% (TARGET)
makerdao-dss-jug · evmasm — wall_time -5.05% (TARGET)
makerdao-dss-jug · ir — wall_time -3.18% (TARGET)
makerdao-dss-pot · evmasm — wall_time -2.16% (TARGET)
makerdao-dss-pot · ir — wall_time -3.42% (TARGET)
makerdao-dss-vat · evmasm — wall_time -3.76% (TARGET)
makerdao-dss-vat · ir — wall_time -3.68% (TARGET)
makerdao-dai · evmasm — wall_time -3.81% (TARGET)
makerdao-dai · ir — wall_time -2.9% (TARGET)
poap · evmasm — wall_time -2.88% (TARGET)
poap · ir — wall_time -3.51% (TARGET)
rocketpool-deposit-pool · evmasm — wall_time -2.4% (TARGET)
rocketpool-deposit-pool · ir — wall_time -3.0% (TARGET)
spark-protocol-pool · evmasm — wall_time -2.85% (TARGET)
spark-protocol-pool · ir — wall_time -3.66% (TARGET)
tether-usdt · evmasm — wall_time -2.77% (TARGET)
tether-usdt · ir — wall_time -3.23% (TARGET)
uniswap-v2-router02 · evmasm — wall_time -3.28% (TARGET)
uniswap-v2-router02 · ir — wall_time -2.02% (~noise)
uniswap-v3-nonfungible-position-manager · evmasm — wall_time -3.13% (TARGET)
uniswap-v3-nonfungible-position-manager · ir — wall_time -3.59% (TARGET)
uniswap-v3-swaprouter02 · evmasm — wall_time -3.34% (TARGET)
uniswap-v3-swaprouter02 · ir — wall_time -2.88% (TARGET)
uniswap-v4-position-manager · evmasm — wall_time -0.63% (TARGET)
uniswap-v4-position-manager · ir — wall_time -0.63% (TARGET)
uniswap-v4-quoter · evmasm — wall_time -0.43% (TARGET)
uniswap-v4-quoter · ir — wall_time -0.4% (TARGET)
wbtc · evmasm — wall_time -0.29% (TARGET)
wbtc · ir — wall_time -0.29% (TARGET)
subsets/bedrock-interfaces-7.0.0 · evmasm — wall_time -0.26% (TARGET)
subsets/bedrock-interfaces-7.0.0 · ir — wall_time -0.62% (TARGET)
subsets/bedrock-analysis-7.0.0 · evmasm — wall_time -0.23% (~noise)
subsets/openzeppelin-timelock-5.6.1 · evmasm — wall_time +0.2% (~noise)
subsets/openzeppelin-timelock-5.6.1 · ir — wall_time +0.58% (~noise)
subsets/aave-pool-3.6.0 · evmasm — wall_time +0.05% (~noise)
subsets/aave-pool-3.6.0 · ir — wall_time -0.21% (~noise)
subsets/solady-libstring-0.1.26 · evmasm — wall_time +0.59% (BASELINE)
subsets/solady-libstring-0.1.26 · ir — wall_time +0.02% (~noise)
subsets/solady-fixedpointmath-0.1.26 · evmasm — wall_time +0.45% (BASELINE)
subsets/solady-fixedpointmath-0.1.26 · ir — wall_time +0.72% (BASELINE)
subsets/v4-poolmanager-4.0.0 · evmasm — wall_time -0.29% (TARGET)
subsets/v4-poolmanager-4.0.0 · ir — wall_time -0.43% (TARGET)
subsets/seaport-core-1.6 · evmasm — wall_time -0.23% (TARGET)
subsets/seaport-core-1.6 · ir — wall_time -0.59% (TARGET)
subsets/bedrock-cannon-7.0.0 · evmasm — wall_time -0.34% (TARGET)
subsets/bedrock-cannon-7.0.0 · ir — wall_time -1.97% (~noise)
subsets/bedrock-l2contractsmanager-7.0.0 · evmasm — wall_time -0.02% (~noise)
subsets/bedrock-l2contractsmanager-7.0.0 · ir — wall_time -0.6% (TARGET)
subsets/bedrock-nutbundle-7.0.0 · evmasm — wall_time -0.03% (~noise)
subsets/bedrock-nutbundle-7.0.0 · ir — wall_time -0.4% (TARGET)
subsets/bedrock-deploy-scripts-7.0.0 · evmasm — wall_time +0.49% (~noise)
subsets/aave-librarypreCompile-3.6.0 · evmasm — wall_time -0.33% (TARGET)
subsets/aave-librarypreCompile-3.6.0 · ir — wall_time -0.25% (TARGET)
subsets/seaport-referenceconsideration-1.6 · evmasm — wall_time -0.25% (TARGET)
subsets/seaport-referenceconsideration-1.6 · ir — wall_time -0.58% (TARGET)
|
rodiazet
force-pushed
the
fix-mstore-removal
branch
from
August 14, 2026 13:15
2110488 to
3c5f8a9
Compare
1 task
rodiazet
force-pushed
the
fix-mstore-removal
branch
from
August 17, 2026 11:46
3c5f8a9 to
c50550e
Compare
rodiazet
changed the base branch from
develop
to
unordered-map-ssa-value-tracker
August 17, 2026 11:47
Co-Authored-By: Nikola Matić <nikola.matic@ethereum.org>
rodiazet
force-pushed
the
fix-mstore-removal
branch
from
August 21, 2026 12:58
c50550e to
d4d524f
Compare
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
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.
SSAValueTrackerimplementation has changed to support also variables which are functions parameters. Previous implementation omitted function parameters because they do not have default value.SSAValueTrackersupported only variables which are initialised, directly in the code or by default value. Functions return variables are set to0, but function parameters have values passed by the caller. Additional set is added to store function parameters which are not assigned in the function body. This change allowed to implement function which filters out variables which (with their dependencies) are not in SSA form. It's needed to properly define a set which serves as an input to the knowledge base.
Fixes: #16458