-
Notifications
You must be signed in to change notification settings - Fork 327
feat(mem2reg): address last known value is independent of its aliases (take three) #9633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20
.
Benchmark suite | Current: e5894e6 | Previous: c6835b5 | Ratio |
---|---|---|---|
test_report_zkpassport_noir-ecdsa_ |
3 s |
1 s |
3 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
It still fails on Aztec Packages... I'll maybe try again tomorrow. |
Reopening because I'll try to fix the issue in this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20
.
Benchmark suite | Current: afb9ae0 | Previous: 3629a25 | Ratio |
---|---|---|---|
private-kernel-reset |
10.284 s |
7.78 s |
1.32 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20
.
Benchmark suite | Current: afb9ae0 | Previous: 3629a25 | Ratio |
---|---|---|---|
sha512-100-bytes |
0.133 s |
0.1 s |
1.33 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
This is now ready for review. CI is green and this PR in Aztec-Packages is green too (AztecProtocol/aztec-packages#16576). |
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix: make Ord for slices lexicographic (elements first, then length) (noir-lang/noir#9555) chore(ssa): Refactor `unrolling` (noir-lang/noir#9653) chore(docs): Update dependency page's examples (noir-lang/noir#9634) fix(ssa): Constant fold Brillig calls using the SSA interpreter (noir-lang/noir#9655) chore: LICM refactors (noir-lang/noir#9642) chore: add test for trait bound on implementing type (noir-lang/noir#9652) chore: pass `DataFlowGraph` instead of `Function` as arg (noir-lang/noir#9656) feat: Group one audit tests (noir-lang/noir#9445) fix(expand): better handling of dereferences (again) (noir-lang/noir#9654) feat(mem2reg): address last known value is independent of its aliases (take three) (noir-lang/noir#9633) chore: remove handling for slice arguments to MSM (noir-lang/noir#9648) fix: validate binary operations which do not allow fields (noir-lang/noir#9649) END_COMMIT_OVERRIDE
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec. BEGIN_COMMIT_OVERRIDE fix: make Ord for slices lexicographic (elements first, then length) (noir-lang/noir#9555) chore(ssa): Refactor `unrolling` (noir-lang/noir#9653) chore(docs): Update dependency page's examples (noir-lang/noir#9634) fix(ssa): Constant fold Brillig calls using the SSA interpreter (noir-lang/noir#9655) chore: LICM refactors (noir-lang/noir#9642) chore: add test for trait bound on implementing type (noir-lang/noir#9652) chore: pass `DataFlowGraph` instead of `Function` as arg (noir-lang/noir#9656) feat: Group one audit tests (noir-lang/noir#9445) fix(expand): better handling of dereferences (again) (noir-lang/noir#9654) feat(mem2reg): address last known value is independent of its aliases (take three) (noir-lang/noir#9633) chore: remove handling for slice arguments to MSM (noir-lang/noir#9648) fix: validate binary operations which do not allow fields (noir-lang/noir#9649) END_COMMIT_OVERRIDE
Description
Problem
Fixes #9468
Summary
Trying to apply #9613 once more, but fixing an issue.
First issue
I think the refactor in #9613 was fine except:
get_known_value
was changed to get an address' value regardless of whether the address has aliases.This
get_known_value
method is only used in one place: when analyzing Load instructions. There, ifget_known_value
returned None we did this:That was actually "because the address might be aliased we better keep the stores to it". That is, because
get_known_value
implied both that the address didn't have aliases while having a known value, it returningNone
potentially meant that it was aliased. Now thatget_known_value
is not related to aliases it's not okay to do that if the value is unknown.Instead, we must preserve stores to the address if the address is potentially aliased, which is what this PR does (so it's further separating the two concepts).
Second issue
get_known_value
retrieving a known value even if an address is aliases, combined with issue #9638, triggered a bug. That was then fixed by #9640Third issue
The fix above worked because
get_known_value
returned nothing if an address has an alias. So in this code:with the second issue fix we end up with v2 being aliased to v0, and v3 being aliased to v1.
Then
load v2
cannot be simplified because it doesn't have a known value (well, it has, but the code in master returnsNone
if it has aliases).Now that
get_known_value
does return a value even if it has aliases, the code above breaks.The issue is that we set v0 as an alias of v2, but we don't also set the aliases of v0 to be aliases of v2. That is, v2 should be aliased to v0 and v1. That's what the last commit does. It doesn't seem to introduce regressions, and in fact this PR is an optimization.
Additional Context
With these three fixes, CI on Aztec-Packages is green: AztecProtocol/aztec-packages#16576 🎉
Documentation
Check one:
PR Checklist
cargo fmt
on default settings.