chore: don't use set_value_from_id in remove_if_else

[asterite]

Based on top of #8039

Description

Problem

Slowly trying to migrate away from set_value_from_id

Summary

Additional Context

The docs for remove_if_else seem to be wrong (copied from another place) but I'm not the best one to document it.

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.

[github-actions]

Changes to circuit sizes

Generated at commit: e1f0b5d62370306fd34068035be111509b8ee017, compared to commit: 6c2fe6da4d711824e73b766635e3d85e6e7879e3

🧾 Summary (10% most significant diffs)

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
conditional_1	+742 ❌	+27.38%	+2,544 ❌	+31.35%
regression_5252	+453 ❌	+1.74%	+1,837 ❌	+2.30%

---

Full diff report 👇

Program	ACIR opcodes (+/-)	%	Circuit size (+/-)	%
conditional_1	3,452 (+742)	+27.38%	10,660 (+2,544)	+31.35%
regression_5252	26,517 (+453)	+1.74%	81,563 (+1,837)	+2.30%

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: 9a1861e	Previous: a6e9d03	Ratio
purely_sequential_opcodes	3277247 ns/iter (± 23165)	265575 ns/iter (± 1628)	12.34
perfectly_parallel_opcodes	3249882 ns/iter (± 49406)	232779 ns/iter (± 4725)	13.96

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

CC: @TomAFrench

[TomAFrench]

Performance regression is a false-alarm and will go away with a merge.

[asterite]

For this program:

fn main(x: u32) {
    let mut array1 = [0 as u32; 2];
    let mut array2 = [0 as u32; 2];

    array1[0] = array2[0];
    if x == 0 {
        array2[0] = 1;
    }

    array1[0] = array2[1];
    if x == 0 {
        array2[1] = 1;
    }
}

The SSA right before "Remove IfElse" is this:

acir(inline) predicate_pure fn main f0 {
  b0(v0: u32):
    v2 = make_array [u32 0, u32 0] : [u32; 2]
    v3 = allocate -> &mut [u32; 2]
    v4 = make_array [u32 0, u32 0] : [u32; 2]
    v5 = allocate -> &mut [u32; 2]
    v6 = make_array [u32 0, u32 0] : [u32; 2]
    v7 = eq v0, u32 0
    enable_side_effects v7
    v9 = make_array [u32 1, u32 0] : [u32; 2]
    v10 = not v7
    v11 = if v7 then v9 else (if v10) v4
    enable_side_effects u1 1
    v13 = array_get v11, index u32 1 -> u32
    v14 = make_array [v13, u32 0] : [u32; 2]
    v15 = eq v0, u32 0
    enable_side_effects v15
    v16 = array_set v11, index u32 1, value u32 1
    v17 = not v15
    v18 = if v15 then v16 else (if v17) v11
    enable_side_effects u1 1
    return
}

Then in master we get:

acir(inline) predicate_pure fn main f0 {
  b0(v0: u32):
    v2 = make_array [u32 0, u32 0] : [u32; 2]
    v3 = allocate -> &mut [u32; 2]
    v4 = make_array [u32 0, u32 0] : [u32; 2]
    v5 = allocate -> &mut [u32; 2]
    v6 = make_array [u32 0, u32 0] : [u32; 2]
    v7 = eq v0, u32 0
    enable_side_effects v7
    v9 = make_array [u32 1, u32 0] : [u32; 2]
    v10 = not v7
    v11 = cast v7 as u32
    v12 = cast v10 as u32
    v13 = make_array [v11, u32 0] : [u32; 2]
    enable_side_effects u1 1
    v15 = array_get v13, index u32 1 -> u32
    v16 = make_array [v15, u32 0] : [u32; 2]
    v17 = eq v0, u32 0
    enable_side_effects v17
    v18 = array_set v13, index u32 1, value u32 1
    v19 = not v17
    v20 = cast v17 as u32
    v21 = cast v19 as u32
    v22 = make_array [v11, v20] : [u32; 2]
    enable_side_effects u1 1
    return
}

and in this PR we get:

acir(inline) predicate_pure fn main f0 {
  b0(v0: u32):
    v2 = make_array [u32 0, u32 0] : [u32; 2]
    v3 = allocate -> &mut [u32; 2]
    v4 = make_array [u32 0, u32 0] : [u32; 2]
    v5 = allocate -> &mut [u32; 2]
    v6 = make_array [u32 0, u32 0] : [u32; 2]
    v7 = eq v0, u32 0
    enable_side_effects v7
    v9 = make_array [u32 1, u32 0] : [u32; 2]
    v10 = not v7
    v11 = cast v7 as u32
    v12 = cast v10 as u32
    v13 = make_array [v11, u32 0] : [u32; 2]
    enable_side_effects u1 1
    v15 = array_get v13, index u32 1 -> u32
    v16 = make_array [v15, u32 0] : [u32; 2]
    v17 = eq v0, u32 0
    enable_side_effects v17
    v18 = array_set v13, index u32 1, value u32 1
    v19 = not v17
    enable_side_effects v17
    v20 = cast v17 as u32
    v21 = cast v19 as u32
    v22 = array_set v18, index u32 1, value v20
    enable_side_effects v17
    enable_side_effects u1 1
    return
}

It seems the difference is that in master there's this:

v22 = make_array [v11, v20] : [u32; 2]

while in this PR there's this:

v22 = array_set v18, index u32 1, value v20

Because the array_set doesn't have mut I think both are equivalent, though I don't understand why the code change here makes that difference. What I'm almost sure is that this PR is more correct because it will replace all occurrences of the old value with the new one, while in master we might not be using resolve all the time (well, that's my hunch). So I feel there might be a bug in master but I don't know how to trigger it if it does exist.

[github-actions]

⚠️ 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: 9a1861e	Previous: a6e9d03	Ratio
private-kernel-inner	0.076 s	0.029 s	2.62
private-kernel-reset	0.296 s	0.163 s	1.82
private-kernel-tail	0.028 s	0.017 s	1.65
rollup-base-private	0.822 s	0.337 s	2.44
rollup-base-public	0.563 s	0.212 s	2.66
rollup-merge	0.007 s	0.004 s	1.75
rollup-root	0.026 s	0.013 s	2

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

CC: @TomAFrench

[github-actions]

⚠️ 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: 9a1861e	Previous: a6e9d03	Ratio
rollup-base-private	22.32 s	17.88 s	1.25

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

CC: @TomAFrench

[jfecher]

This looks like the try_merge_only_changed_indices optimization is being applied to this change where on master we fell back to creating a new array from each index. For an array of length 2 it doesn't really matter but for larger arrays it should be beneficial.

[asterite]

@jfecher Thanks, you are right! I just checked and in master the try_merge_only_changed_indices branch is never hit, while it is in this PR. I checked where's the difference and if in master I resolve the values in this line before using them then try_merge_only_changed_indices is hit. That matches my intuition that resolve might be missing from some places.

So, if I'm understanding things correctly, this PR/change is good despite the regressions?

[jfecher]

So, if I'm understanding things correctly, this PR/change is good despite the regressions?

It sounded like from Tom that these may go away after a merge? Or was that just for the 13x performance alert? Let's merge master and re-evaluate.

Edit: Ok, just the 13x then. Looks like it didn't go away after the merge in 9a1861e though

[TomAFrench]

Yeah, just the 13x performance regression.

[jfecher]

Since the CI for this PR is a bit messed up, can you make sure to manually test this against a few aztec crates as well?

[asterite]

Sure! When you say "manually test" is that in a different way than the external checks here? I guess it's to compare the sizes and so on. Is there a script I can run to perform the same checks/reports?

[jfecher]

I usually just run nargo on these crates manually. It shouldn't be different than the external checks - just noting down e.g. any brillig size or acir constraint count differences versus master. I do it sometimes when I don't trust the CI.

[asterite]

I just checked and there's a regression for rollup_base_private ACIR opcodes:

• in master it's 272017
• this PR with the try_merge_only_changed_indices optimization: 272125
• this PR without that optimization: 275025

So it seems that optimization is good but maybe for the programs in test_programs it makes things worse.

That said, I'm still not sure why it still regresses a bit in this PR.

Some other crates I checked stay the same.

[asterite]

I'll close this PR and create a new one once the base PR is merged.