fix: Fix poor handling of aliased references in flattening pass causing some values to be zeroed #6434
Conversation
This file contains 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
|
|
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
jfecher
commented
Nov 4, 2024
Comment on lines
+385
to
+387
| // if let Some(last_store) = references.last_stores.get(&address) { | ||
| // self.instructions_to_remove.insert(*last_store); | ||
| // } |
There was a problem hiding this comment.
Had to comment out this portion of mem2reg as it was causing the sha256 tests to fail. There seems to be an issue with tracking last_stores for some reason now.
Comment on lines
390
to
392
| // Store instructions must be removed by DIE in acir code, any load | ||
| // instructions should already be unused by that point. | ||
| Store { .. } => matches!(function.runtime(), RuntimeType::Acir(_)), |
There was a problem hiding this comment.
With the change to mem2reg, stores weren't being removed otherwise for some reason (even though the comment claims to just remove certain unneeded stores). So I had to change DIE to remove all stores here.
|
The brillig size increases are likely from the temporary store changes while mem2reg is fixed (should it be in this PR or separate?) |
…with-merger * tf/experiment-with-alternative-merger: . feat: avoid branch condition negations in `ValueMerger`
5 tasks
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.
Description
Problem*
Resolves #5771
Summary*
Background
Fixes a bug which originates from
merge_storesin the flattening pass which can in certain cases get confused with aliases and overwrite values incorrectly.The rough steps to reproduce this issue are as follows, though I've been unsuccessful so far in reproducing it from scratch outside of the original branch in aztec packages.
context.side_effect_counterin the original bug)if input_to_main { ... }, and ensure thatinput_to_main = falseThis somewhat specific setup lead to a situation where the flattening pass saw it had a store to what it saw as two different references, but were really both aliases to the same reference.
merge_storesuses athen_value,else_value, andold_valueof each reference to create itsnew_valuewhich it then stores back to the reference. One alias in this case had a correctthen_value, while the other had a correctelse_value, with the opposite value of both being equal to theold_value. Storing both of these in sequence lead to the final value for the else/false case to be!input_to_main * (input_to_main * else_value). Since one ofinput_to_mainand its negation are always zero, the whole value ends up zeroed for the else case. Finally, confusion with aliases also leads the compiler to accept the else case value as the updated value even after theifterminates.I've been unsuccessful trying to reproduce this from scratch so come Monday I'll work on copying over the same test case into an integration test here. I've already removed all macro code but need to figure out how to translate the oracle functions step mainly.
The Fix
Skipping some alternate fixes I tried, I figured out we could just remove
merge_storesentirely if we never stored a non-conditional value to a reference to begin with during flattening. E.g. currently in flattening if we seeif c { foo = 3; }we issue astore 3 in foothen after the if we go back and merge stores to issue astore (if c then 3 else prev_value) in fooafterward. I've changed it to instead issueprev_value = load foo; store (if c then 3 else prev_value) in footo replace the original store. This means the value of a reference is always correct and we never have to fix it afterward. This greatly simplifies the flattening pass I think.HOWEVER:
foo = allocate; store 3 in foo, so I had to add aninitial_value: ValueIdargument to the Allocate instruction.Instruction::IfElsefor every store leads to more value merging in practice, resulting in increase constraint counts without something like feat: avoid branch condition negations inValueMerger#6073.Additional Context
Draft because I had to port this over from local changes within aztec packages (branch jf/fix-zeroed-bug) and there are a few merges that still need to be handled carefully - most notably the mem2reg changes.
Remaining work
Instruction::Allocatechanges in favor of hackDocumentation*
Check one:
PR Checklist*
cargo fmton default settings.