Skip to content

fix: Fix if-else alias in mem2reg#9611

Merged
jfecher merged 3 commits intomasterfrom
jf/fix-if-else-mem2reg
Aug 22, 2025
Merged

fix: Fix if-else alias in mem2reg#9611
jfecher merged 3 commits intomasterfrom
jf/fix-if-else-mem2reg

Conversation

@jfecher
Copy link
Contributor

@jfecher jfecher commented Aug 21, 2025

Description

Problem*

Resolves #9464

Summary*

After an if-else instruction, then_value and else_value should both be aliased to the resulting value but this wasn't handled at all previously.

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.

@jfecher jfecher requested a review from a team August 21, 2025 20:17
@github-actions
Copy link
Contributor

github-actions bot commented Aug 21, 2025

Changes to Brillig bytecode sizes

Generated at commit: 2f757ae72663b169236444e4ace5501668356bf1, compared to commit: 10a597f42aca9d2dbb9ab31e9343b0189e879671

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
regression_9303_inliner_max +1 ❌ +0.95%
regression_9303_inliner_min +1 ❌ +0.95%
regression_9303_inliner_zero +1 ❌ +0.95%

Full diff report 👇
Program Brillig opcodes (+/-) %
regression_9303_inliner_max 106 (+1) +0.95%
regression_9303_inliner_min 106 (+1) +0.95%
regression_9303_inliner_zero 106 (+1) +0.95%

@aakoshh
Copy link
Contributor

aakoshh commented Aug 21, 2025

So previously I set the result to have the then_value and the end_value as aliases, and now it goes the other way as well. I asked a couple of times why aliasing wasn’t bidirectional. Or it is, but we have to remember to manually code it every time?

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.

Nice!

This is something we were talking about, or we noticed, a few days ago me and Akosh while pairing: that if you make b an alias of a, it doesn't automatically make a an alias of b (I think?). And here it was likely missing, and in fact that was the case. I wonder if that is also missing in other cases...

@asterite
Copy link
Collaborator

Ah, I mentioned the above because I wonder if there's a different way to represent these alias sets...

@github-actions
Copy link
Contributor

github-actions bot commented Aug 21, 2025

Changes to number of Brillig opcodes executed

Generated at commit: 2f757ae72663b169236444e4ace5501668356bf1, compared to commit: 10a597f42aca9d2dbb9ab31e9343b0189e879671

🧾 Summary (10% most significant diffs)

Program Brillig opcodes (+/-) %
regression_9303_inliner_max +2 ❌ +1.26%
regression_9303_inliner_min +2 ❌ +1.26%
regression_9303_inliner_zero +2 ❌ +1.26%

Full diff report 👇
Program Brillig opcodes (+/-) %
regression_9303_inliner_max 161 (+2) +1.26%
regression_9303_inliner_min 161 (+2) +1.26%
regression_9303_inliner_zero 161 (+2) +1.26%

@aakoshh
Copy link
Contributor

aakoshh commented Aug 21, 2025

It's possible that with this, #9567 would not be required on the load side.

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: dc312d9 Previous: 10a597f Ratio
test_report_zkpassport_noir-ecdsa_ 3 s 2 s 1.50
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
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 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: b59e207 Previous: 10a597f Ratio
private-kernel-inner 0.015 s 0.012 s 1.25

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

CC: @TomAFrench

@jfecher
Copy link
Contributor Author

jfecher commented Aug 21, 2025

So previously I set the result to have the then_value and the end_value as aliases, and now it goes the other way as well. I asked a couple of times why aliasing wasn’t bidirectional. Or it is, but we have to remember to manually code it every time?

It's because the way aliasing is handled it isn't quite bidirectional.

Using this as an example, if we have v3 = if v0 then v1 else v2 where v0: u1 and all others are references, then we know:

  • v3 aliases v1 or v2 (conservatively, we say it aliases both)
  • v1 is (conservatively) aliased by v3
  • v2 is (conservatively) aliased by v3

So if we see:

store Field 0 in v3
v5 = load v1 -> Field

We can't simplify out the load in case v3 refers to it.
However, we can simplify it out if the load refers to the same reference:

store Field 0 in v3
v5 = load v3 -> Field

In this case v3 is still aliased to both v1 and v2 yet v3 (and only v3) has a known value.

I am doubting our representation of this all though. E.g. maybe the aliases list should be more separate from whether a value is known? Such that the alias list is shared but we still track known values per-reference? The Expression enum is also maybe underused and seems questionable (can we always identify which Expression a value id should refer to?) and we also track aliases outside of the Block structure in aliased_references.

@asterite
Copy link
Collaborator

Such that the alias list is shared but we still track known values per-reference?

That's a good point. It seems we do track known values per-reference, in the references field of a Block. However, when we go to fetch an address' known value, if that references has aliases then we say we don't know what that value is. Also when we set the value of an address to point to a ReferenceValue we might either clear out all references or clear out all aliases, but we never remember that for this particular address we know the value. See #9468

I'll try a PR changing that to see if it works. It would at least decouple references from expressions and aliases a bit.

@jfecher
Copy link
Contributor Author

jfecher commented Aug 22, 2025

Ah - one more thing. This is maybe obvious but I'll mention it anyway.

Even if we want setting an alias to be bidirectional (so that when we alias v3 with v1 and v2 above, v1 and v2 are also aliased with v3), this does not mean they actually share the same alias list. Since v3 may alias either, but v1 is still known not to alias v2 and vice-versa.

@jfecher jfecher added this pull request to the merge queue Aug 22, 2025
Merged via the queue into master with commit f404d69 Aug 22, 2025
122 checks passed
@jfecher jfecher deleted the jf/fix-if-else-mem2reg branch August 22, 2025 13:37
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: store lost after returning mutable reference from if

3 participants