Skip to content

chore(mem2reg): Only add to per function last_loads if load is not removed#9647

Merged
asterite merged 2 commits intomasterfrom
mv/only-add-last-loads-per-func-if-not-removed
Aug 26, 2025
Merged

chore(mem2reg): Only add to per function last_loads if load is not removed#9647
asterite merged 2 commits intomasterfrom
mv/only-add-last-loads-per-func-if-not-removed

Conversation

@vezenovm
Copy link
Contributor

Description

Problem*

Resolves #9469

Summary*

As per the issue description, this case should be unlikely but we should be clear about the intention of our logic. PerFunctionContext::last_loads should only containing loads that have survived. Previously we could potentially have a load that returns None from get_known_value thus inserting that load address into PerFunctionContext::last_loads even though it could potentially be later removed by looking at references.last_loads.

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@vezenovm vezenovm requested a review from a team August 26, 2025 19:35
@asterite
Copy link
Collaborator

this case should be unlikely

In the PR where I'm trying to reapply the PR that we reverted I had to make some changes around there. It might be that this PR will trivially fix the issues I was having, but we'll see.

https://github.com/noir-lang/noir/pull/9633/files#diff-9cbe261da2bf371c7c0127b7c8780f5dcdc6a5782f64951bf75953feb7115f6bL448-L449

Copy link
Contributor

@github-actions github-actions bot left a 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: 5fb715d Previous: 6870579 Ratio
test_report_zkpassport_noir_rsa_ 2 s 1 s 2

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Collaborator

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@asterite asterite added this pull request to the merge queue Aug 26, 2025
Merged via the queue into master with commit 7c31a63 Aug 26, 2025
122 checks passed
@asterite asterite deleted the mv/only-add-last-loads-per-func-if-not-removed branch August 26, 2025 21:58
AztecBot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 27, 2025
Automated pull of nightly from the [noir](https://github.com/noir-lang/noir) programming language, a dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: don't thread-bomb unnecessarily (noir-lang/noir#9643)
chore(mem2reg): Only add to per function last_loads if load is not removed (noir-lang/noir#9647)
chore(mem2reg): add a few regression tests (noir-lang/noir#9615)
fix: Monomorphize function values as pairs of `(constrained, unconstrained)` (noir-lang/noir#9484)
fix(mem2reg): Mark block parameters with unknown alias sets in presence of nested references  (noir-lang/noir#9629)
fix(ssa): Put some default in `Value::uninitialized` for references in the SSA interpreter (noir-lang/noir#9603)
feat(ssa_fuzzer): ecdsa blackbox functions (noir-lang/noir#9584)
fix(mem2reg): missing alias from block parameter to its argument (noir-lang/noir#9640)
fix(acir_gen): A slice might be a nested Array, not a flattened DynamicArray (noir-lang/noir#9600)
chore: add another mem2reg regression for #9613 (noir-lang/noir#9635)
chore: document remove_if_else (in preparation for audit) (noir-lang/noir#9621)
fix(mem2reg): Assume all function reference parameters have an unknown alias set with nested references (noir-lang/noir#9632)
chore: add a regression test for #9613 (noir-lang/noir#9630)
fix: Revert "feat(mem2reg): address last known value is independent of its… (noir-lang/noir#9628)
fix(inlining): Do not inline globals and lower them during ACIR gen (noir-lang/noir#9626)
chore(brillig): Include function name with `--count-array-copies` debug information (noir-lang/noir#9623)
chore: use `assert_ssa_does_not_change` throughout all SSA tests (noir-lang/noir#9625)
chore: only run remove_paired_rc in brillig functions (noir-lang/noir#9624)
chore: some inlining refactors (noir-lang/noir#9622)
chore(mem2reg): avoid redundant PostOrder computation (noir-lang/noir#9620)
chore: bump external pinned commits (noir-lang/noir#9618)
chore: document intrinsics (noir-lang/noir#9382)
fix: Make inc/dec_rc impure (noir-lang/noir#9617)
chore: greenlight `checked_to_unchecked` for audits (noir-lang/noir#9537)
feat(mem2reg): address last known value is independent of its aliases (noir-lang/noir#9613)
fix: Fix if-else alias in mem2reg (noir-lang/noir#9611)
END_COMMIT_OVERRIDE
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 27, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: don't thread-bomb unnecessarily
(noir-lang/noir#9643)
chore(mem2reg): Only add to per function last_loads if load is not
removed (noir-lang/noir#9647)
chore(mem2reg): add a few regression tests
(noir-lang/noir#9615)
fix: Monomorphize function values as pairs of `(constrained,
unconstrained)` (noir-lang/noir#9484)
fix(mem2reg): Mark block parameters with unknown alias sets in presence
of nested references (noir-lang/noir#9629)
fix(ssa): Put some default in `Value::uninitialized` for references in
the SSA interpreter (noir-lang/noir#9603)
feat(ssa_fuzzer): ecdsa blackbox functions
(noir-lang/noir#9584)
fix(mem2reg): missing alias from block parameter to its argument
(noir-lang/noir#9640)
fix(acir_gen): A slice might be a nested Array, not a flattened
DynamicArray (noir-lang/noir#9600)
chore: add another mem2reg regression for #9613
(noir-lang/noir#9635)
chore: document remove_if_else (in preparation for audit)
(noir-lang/noir#9621)
fix(mem2reg): Assume all function reference parameters have an unknown
alias set with nested references
(noir-lang/noir#9632)
chore: add a regression test for #9613
(noir-lang/noir#9630)
fix: Revert "feat(mem2reg): address last known value is independent of
its… (noir-lang/noir#9628)
fix(inlining): Do not inline globals and lower them during ACIR gen
(noir-lang/noir#9626)
chore(brillig): Include function name with `--count-array-copies` debug
information (noir-lang/noir#9623)
chore: use `assert_ssa_does_not_change` throughout all SSA tests
(noir-lang/noir#9625)
chore: only run remove_paired_rc in brillig functions
(noir-lang/noir#9624)
chore: some inlining refactors
(noir-lang/noir#9622)
chore(mem2reg): avoid redundant PostOrder computation
(noir-lang/noir#9620)
chore: bump external pinned commits
(noir-lang/noir#9618)
chore: document intrinsics (noir-lang/noir#9382)
fix: Make inc/dec_rc impure
(noir-lang/noir#9617)
chore: greenlight `checked_to_unchecked` for audits
(noir-lang/noir#9537)
feat(mem2reg): address last known value is independent of its aliases
(noir-lang/noir#9613)
fix: Fix if-else alias in mem2reg
(noir-lang/noir#9611)
END_COMMIT_OVERRIDE
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 27, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: don't thread-bomb unnecessarily
(noir-lang/noir#9643)
chore(mem2reg): Only add to per function last_loads if load is not
removed (noir-lang/noir#9647)
chore(mem2reg): add a few regression tests
(noir-lang/noir#9615)
fix: Monomorphize function values as pairs of `(constrained,
unconstrained)` (noir-lang/noir#9484)
fix(mem2reg): Mark block parameters with unknown alias sets in presence
of nested references (noir-lang/noir#9629)
fix(ssa): Put some default in `Value::uninitialized` for references in
the SSA interpreter (noir-lang/noir#9603)
feat(ssa_fuzzer): ecdsa blackbox functions
(noir-lang/noir#9584)
fix(mem2reg): missing alias from block parameter to its argument
(noir-lang/noir#9640)
fix(acir_gen): A slice might be a nested Array, not a flattened
DynamicArray (noir-lang/noir#9600)
chore: add another mem2reg regression for #9613
(noir-lang/noir#9635)
chore: document remove_if_else (in preparation for audit)
(noir-lang/noir#9621)
fix(mem2reg): Assume all function reference parameters have an unknown
alias set with nested references
(noir-lang/noir#9632)
chore: add a regression test for #9613
(noir-lang/noir#9630)
fix: Revert "feat(mem2reg): address last known value is independent of
its… (noir-lang/noir#9628)
fix(inlining): Do not inline globals and lower them during ACIR gen
(noir-lang/noir#9626)
chore(brillig): Include function name with `--count-array-copies` debug
information (noir-lang/noir#9623)
chore: use `assert_ssa_does_not_change` throughout all SSA tests
(noir-lang/noir#9625)
chore: only run remove_paired_rc in brillig functions
(noir-lang/noir#9624)
chore: some inlining refactors
(noir-lang/noir#9622)
chore(mem2reg): avoid redundant PostOrder computation
(noir-lang/noir#9620)
chore: bump external pinned commits
(noir-lang/noir#9618)
chore: document intrinsics (noir-lang/noir#9382)
fix: Make inc/dec_rc impure
(noir-lang/noir#9617)
chore: greenlight `checked_to_unchecked` for audits
(noir-lang/noir#9537)
feat(mem2reg): address last known value is independent of its aliases
(noir-lang/noir#9613)
fix: Fix if-else alias in mem2reg
(noir-lang/noir#9611)
END_COMMIT_OVERRIDE
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Aug 27, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
fix: don't thread-bomb unnecessarily
(noir-lang/noir#9643)
chore(mem2reg): Only add to per function last_loads if load is not
removed (noir-lang/noir#9647)
chore(mem2reg): add a few regression tests
(noir-lang/noir#9615)
fix: Monomorphize function values as pairs of `(constrained,
unconstrained)` (noir-lang/noir#9484)
fix(mem2reg): Mark block parameters with unknown alias sets in presence
of nested references (noir-lang/noir#9629)
fix(ssa): Put some default in `Value::uninitialized` for references in
the SSA interpreter (noir-lang/noir#9603)
feat(ssa_fuzzer): ecdsa blackbox functions
(noir-lang/noir#9584)
fix(mem2reg): missing alias from block parameter to its argument
(noir-lang/noir#9640)
fix(acir_gen): A slice might be a nested Array, not a flattened
DynamicArray (noir-lang/noir#9600)
chore: add another mem2reg regression for #9613
(noir-lang/noir#9635)
chore: document remove_if_else (in preparation for audit)
(noir-lang/noir#9621)
fix(mem2reg): Assume all function reference parameters have an unknown
alias set with nested references
(noir-lang/noir#9632)
chore: add a regression test for #9613
(noir-lang/noir#9630)
fix: Revert "feat(mem2reg): address last known value is independent of
its… (noir-lang/noir#9628)
fix(inlining): Do not inline globals and lower them during ACIR gen
(noir-lang/noir#9626)
chore(brillig): Include function name with `--count-array-copies` debug
information (noir-lang/noir#9623)
chore: use `assert_ssa_does_not_change` throughout all SSA tests
(noir-lang/noir#9625)
chore: only run remove_paired_rc in brillig functions
(noir-lang/noir#9624)
chore: some inlining refactors
(noir-lang/noir#9622)
chore(mem2reg): avoid redundant PostOrder computation
(noir-lang/noir#9620)
chore: bump external pinned commits
(noir-lang/noir#9618)
chore: document intrinsics (noir-lang/noir#9382)
fix: Make inc/dec_rc impure
(noir-lang/noir#9617)
chore: greenlight `checked_to_unchecked` for audits
(noir-lang/noir#9537)
feat(mem2reg): address last known value is independent of its aliases
(noir-lang/noir#9613)
fix: Fix if-else alias in mem2reg
(noir-lang/noir#9611)
END_COMMIT_OVERRIDE
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mem2reg: Last loads per function not accounting for last loads removed per block

2 participants