fix: Thread errors through remove_if_else instead of panicing when the value merger finds reference values

[jfecher]

Description

Problem*

Working towards #8741
Unfortunately this does not 100% solve the issue. We issue a RuntimeError now but panic when reporting the error since the call stack is empty. It turns out none of the call stacks for then_value, else_value, then_condition, or else_condition have any call stacks. This PR is already quite large though so I thought I'd keep this one focused on just error handling.

Summary*

These changes are all just threading RuntimeError through more places in SSA to account for the fact we can now error when inserting instructions. The above is still true but we only thread through remove_if_else now and fail to optimize slice_push_back instead of propagating up further.

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.

[github-actions]

⚠️ 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: 1e081e1	Previous: c2cedd4	Ratio
test_report_zkpassport_noir_rsa_	2 s	1 s	2

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

CC: @TomAFrench

[aakoshh]

Just wanted to mention #8748 as well as being relevant to this.

Can you reiterate the end goal? As I understand you didn't want to deny the ability to reassign mutable references in if statements, so the SSA has to handle it, right? What is supposed to happen with these errors that now returned by all of these passes? Are they just becoming a compilation error caught by the backend, rather than the frontend?

[aakoshh]

Out of curiosity: what stops the frontend from rejecting reassigning mutable references only if they happen in the context of an if statement?

[jfecher]

What is supposed to happen with these errors that now returned by all of these passes? Are they just becoming a compilation error caught by the backend, rather than the frontend?

Yes, it will be reported where we report other SSA errors like when we fail to unroll a loop because the bounds aren't constant.

Out of curiosity: what stops the frontend from rejecting reassigning mutable references only if they happen in the context of an if statement?

We'd have to do it for match too (this was something I missed in the original monomorphization error PR too) but I'm not sure. It just didn't sit right with me at the end of the day. Banning them from returning references directly is one thing but banning any mutable variable containing a reference from being reassigned to within an if/match is much closer to banning all mutable references within mutable variables in general. Only erroring in SSA is a little more lenient in that we're still allowing them in ifs/matches which can be removed at codegen time but is still somewhat restrictive.

Maybe we should bite the bullet and ban mutable references in mutable variables in general? But I'm also not confident this is the core issue. It's more likely to do with nested mutable references in general. I wonder how much code would break if we did this.

I'll add an error during monomorphization today in another PR to ban nested mutable references and mutable refs in mutable vars and see how much code breaks in that PR.

[github-actions]

ACVM Benchmarks

Details

Benchmark suite	Current: 9bfd03d	Previous: 53cf56f	Ratio
purely_sequential_opcodes	249302 ns/iter (± 744)	248762 ns/iter (± 892)	1.00
perfectly_parallel_opcodes	217753 ns/iter (± 3166)	218454 ns/iter (± 9470)	1.00
perfectly_parallel_batch_inversion_opcodes	2777365 ns/iter (± 4779)	2780642 ns/iter (± 1778)	1.00

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

[github-actions]

Artifact Size

Details

Benchmark suite	Current: 9bfd03d	Previous: 53cf56f	Ratio
private-kernel-inner	1133.6 KB	1133.6 KB	1
private-kernel-reset	2070.7 KB	2070.7 KB	1
private-kernel-tail	587.9 KB	587.9 KB	1
rollup-base-private	4957.3 KB	4957.3 KB	1
rollup-base-public	4003.5 KB	4003.5 KB	1
rollup-block-root-empty	258.9 KB	258.9 KB	1
rollup-block-root-single-tx	25707.2 KB	25707.2 KB	1
rollup-block-root	25722.8 KB	25722.8 KB	1
rollup-merge	185.1 KB	185.1 KB	1
rollup-root	419.4 KB	419.4 KB	1
semaphore-depth-10	636.4 KB	636.4 KB	1

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

[github-actions]

Compilation Time

Details

Benchmark suite	Current: 9bfd03d	Previous: 53cf56f	Ratio
private-kernel-inner	2.324 s	2.354 s	0.99
private-kernel-reset	7.324 s	7.446 s	0.98
private-kernel-tail	1.12 s	1.054 s	1.06
rollup-base-private	15.82 s	15.28 s	1.04
rollup-base-public	13.88 s	13.92 s	1.00
rollup-block-root-empty	1.282 s	1.432 s	0.90
rollup-block-root-single-tx	120 s	120 s	1
rollup-block-root	126 s	131 s	0.96
rollup-merge	1.178 s	1.07 s	1.10
rollup-root	1.586 s	1.566 s	1.01
semaphore-depth-10	0.798 s	0.756 s	1.06

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

[github-actions]

Execution Time

Details

Benchmark suite	Current: 9bfd03d	Previous: 53cf56f	Ratio
private-kernel-inner	0.027 s	0.027 s	1
private-kernel-reset	0.157 s	0.157 s	1
private-kernel-tail	0.011 s	0.011 s	1
rollup-base-private	0.291 s	0.294 s	0.99
rollup-base-public	0.189 s	0.186 s	1.02
rollup-block-root	11 s	11.4 s	0.96
rollup-merge	0.004 s	0.003 s	1.33
rollup-root	0.009 s	0.009 s	1
semaphore-depth-10	0.02 s	0.019 s	1.05

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

[github-actions]

Compilation Memory

Details

Benchmark suite	Current: 9bfd03d	Previous: 53cf56f	Ratio
private-kernel-inner	292.97 MB	292.97 MB	1
private-kernel-reset	534.03 MB	534.03 MB	1
private-kernel-tail	191.55 MB	191.55 MB	1
rollup-base-private	1380 MB	1380 MB	1
rollup-base-public	1550 MB	1550 MB	1
rollup-block-root-empty	343.08 MB	343.07 MB	1.00
rollup-block-root-single-tx	7830 MB	7830 MB	1
rollup-block-root	7830 MB	7830 MB	1
rollup-merge	327.31 MB	327.3 MB	1.00
rollup-root	381.65 MB	381.63 MB	1.00
semaphore_depth_10	106.39 MB	106.39 MB	1

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

[github-actions]

Execution Memory

Details

Benchmark suite	Current: 9bfd03d	Previous: 53cf56f	Ratio
private-kernel-inner	202.1 MB	202.1 MB	1
private-kernel-reset	225.82 MB	225.82 MB	1
private-kernel-tail	177.36 MB	177.36 MB	1
rollup-base-private	489.98 MB	489.98 MB	1
rollup-base-public	423.33 MB	423.33 MB	1
rollup-block-root	1400 MB	1400 MB	1
rollup-merge	312.18 MB	312.18 MB	1
rollup-root	317.87 MB	317.87 MB	1
semaphore_depth_10	70.96 MB	70.96 MB	1

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

[github-actions]

Test Suite Duration

Details

Benchmark suite	Current: 9bfd03d	Previous: 53cf56f	Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr	68 s	70 s	0.97
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts	113 s	115 s	0.98
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob	44 s	43 s	1.02
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib	203 s	210 s	0.97
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib	262 s	250 s	1.05
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types	73 s	74 s	0.99
test_report_noir-lang_noir-bignum_	372 s	378 s	0.98
test_report_noir-lang_noir_bigcurve_	223 s	277 s	0.81
test_report_noir-lang_sha512_	31 s	30 s	1.03
test_report_zkpassport_noir_rsa_	1 s	1 s	1

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

[jfecher]

This PR doesn't seem to slow down our SSA passes so let's merge it to remove the panic I think. In case my monomorphization PR doesn't catch all errors (e.g. nested references without a reference in the rhs: **foo = 3;) this will stop SSA from panicking in those cases and in cases where e.g. a fuzzer or ssa parser produces bad ssa directly.

[asterite]

Like Akosh, I wonder if this could be somehow detected by the frontend.

For example this code in Rust, which is the snippet that's in #8741

fn main() {
    let c = true;
    let mut e: &mut bool =
        &mut false;
    if c {
        e = &mut true;
    };
    println!("{}", *e);
}

Gives this error:

error[E0716]: temporary value dropped while borrowed
 --> src/main.rs:6:18
  |
6 |         e = &mut true;
  |                  ^^^^- temporary value is freed at the end of this statement
  |                  |
  |                  creates a temporary value which is freed while still in use
7 |     };
8 |     println!("{}", *e);
  |                    -- borrow later used here
  |
  = note: consider using a `let` binding to create a longer lived value

Now I know that implementing this might mean we'd need something like the borrow checker, so I understand why we don't do that right now.

I was wondering... it seems the error only happens in merge_values. That's used in two places:

• the remove_if_else optimization
• in simplify_slice_push_back

I wonder if we could then:

• have merge_values return a Result, like it is now
• change SSA passes to return a Result, like now
• in simplify_slice_push_back, if merge_values doesn't return Ok, we don't simplify the instruction. Then I guess it will eventually error somewhere else (not sure!)

At least that way insert_instruction and friends don't need to change to return Result.

[jfecher]

Like Akosh, I wonder if this could be somehow detected by the frontend.

@asterite there is a related PR here: #8790 for erroring in the frontend during monomorphization. Rust's lifetime error is unrelated to the issue here. Rust doesn't have a flattening pass like we do and if it needed to merge pointer values for some reason in its IR it could potentially treat them as integers and merge the integers. Our backend doesn't support actual pointers as values though so we have to remove them before then.

[vezenovm]

The changes to thread errors look fine as is. However, I am wondering whether we could do this as part of a pre post-flattening validation pass as to avoid thread an RtResult<ValueId> from merge_values. We would then only have to change flatten_cfg(mut self) -> RtResult<Ssa>.

[jfecher]

@vezenovm would that require adding unwraps? My worry for adding unwraps to prevent Results when adding instructions is that if results are ever added in more places after it'd be more difficult to track if we're still meeting the conditions to not implicitly panic

[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: 9bfd03d	Previous: 53cf56f	Ratio
rollup-merge	0.004 s	0.003 s	1.33

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

CC: @TomAFrench

[vezenovm]

@vezenovm would that require adding unwraps? My worry for adding unwraps to prevent Results when adding instructions is that if results are ever added in more places after it'd be more difficult to track if we're still meeting the conditions to not implicitly panic

I don't think it would. I need to correct my comment earlier. A post flattening (or pre remove_if_else) validation pass I think would be better. As @asterite pointed merge_values is only ever called in remove_if_else and during the slice push back simplification. After flattening we could run a deeper validation of IfElse that we are not attempting to merge a reference. Only the new post flattening validation pass and flatten_cfg would have to return a Result then.

[jfecher]

Updated using the "don't optimize slice_push_back if we get an error" approach. It's much smaller now. Only the remove_if_else exposes an RtError now as expected (I kept the new type alias).

[vezenovm]

Looks good! It is nice to move away from threading errors through instruction insertion and everywhere else downstream. Probably want to rename the PR as well.