chore: don't compute used globals during DIE

[asterite]

Description

Problem

No issue, just something I noticed while working on #9023

Summary

Before this PR, the Ssa used_globals field was computed, and re-computed, on each DIE pass. I guess it was done like that because DIE also keeps track of used values, so the set of used globals ends up being the set of used values that are globals.

The main issue with the previous approach is that if a pass after the last DIE ended up reusing a global (make_constant can do that) then the of used_globals will be incorrect. To fix this we'd need to run DIE once again, but it's unclear that you have to do this. With this PR everything works fine automatically.

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]

ACVM Benchmarks

Details

Benchmark suite	Current: 846700b	Previous: 8e01c4a	Ratio
purely_sequential_opcodes	251987 ns/iter (± 450)	253431 ns/iter (± 5124)	0.99
perfectly_parallel_opcodes	222313 ns/iter (± 4708)	223414 ns/iter (± 2492)	1.00
perfectly_parallel_batch_inversion_opcodes	2795907 ns/iter (± 5137)	2780689 ns/iter (± 15489)	1.01

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

[github-actions]

Artifact Size

Details

Benchmark suite	Current: 846700b	Previous: 8e01c4a	Ratio
private-kernel-inner	1135.2 KB	1135.2 KB	1
private-kernel-reset	2072.4 KB	2072.4 KB	1
private-kernel-tail	589.1 KB	589.1 KB	1
rollup-base-private	4934 KB	4934 KB	1
rollup-base-public	3974.6 KB	3974.6 KB	1
rollup-block-root-empty	3881 KB	3881 KB	1
rollup-block-root-single-tx	32731.4 KB	32731.4 KB	1
rollup-block-root	32766.2 KB	32766.2 KB	1
rollup-merge	185.8 KB	185.8 KB	1
rollup-root	410.2 KB	410.2 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]

Execution Time

Details

Benchmark suite	Current: 846700b	Previous: 8e01c4a	Ratio
private-kernel-inner	0.027 s	0.027 s	1
private-kernel-reset	0.162 s	0.163 s	0.99
private-kernel-tail	0.011 s	0.012 s	0.92
rollup-base-private	0.295 s	0.292 s	1.01
rollup-base-public	0.186 s	0.185 s	1.01
rollup-block-root	12.9 s	13.4 s	0.96
rollup-merge	0.004 s	0.004 s	1
rollup-root	0.005 s	0.005 s	1
semaphore-depth-10	0.025 s	0.025 s	1

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

[github-actions]

Compilation Time

Details

Benchmark suite	Current: 846700b	Previous: 8e01c4a	Ratio
private-kernel-inner	2.316 s	2.252 s	1.03
private-kernel-reset	7.762 s	7.522 s	1.03
private-kernel-tail	1.096 s	1.12 s	0.98
rollup-base-private	17.34 s	16.7 s	1.04
rollup-base-public	13.88 s	13.5 s	1.03
rollup-block-root-empty	19.52 s	19.88 s	0.98
rollup-block-root-single-tx	184 s	188 s	0.98
rollup-block-root	178 s	184 s	0.97
rollup-merge	1.308 s	1.268 s	1.03
rollup-root	1.332 s	1.33 s	1.00
semaphore-depth-10	0.788 s	0.762 s	1.03

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

[github-actions]

Test Suite Duration

Details

Benchmark suite	Current: 846700b	Previous: 3838c69	Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr	83 s	75 s	1.11
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts	126 s	127 s	0.99
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob	149 s	141 s	1.06
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib	205 s	204 s	1.00
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib	591 s
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types	73 s	73 s	1
test_report_noir-lang_noir-bignum_	376 s
test_report_noir-lang_noir_bigcurve_	246 s
test_report_noir-lang_sha512_	18 s	15 s	1.20
test_report_zkpassport_noir-ecdsa_	99 s	115 s	0.86
test_report_zkpassport_noir_rsa_	1 s	2 s	0.50

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

[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: 5a2a30a	Previous: 3838c69	Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob	209 s	141 s	1.48

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

CC: @TomAFrench

[github-actions]

Compilation Memory

Details

Benchmark suite	Current: 846700b	Previous: 8e01c4a	Ratio
private-kernel-inner	293.26 MB	293.27 MB	1.00
private-kernel-reset	533.9 MB	533.9 MB	1
private-kernel-tail	191.88 MB	191.86 MB	1.00
rollup-base-private	1400 MB	1400 MB	1
rollup-base-public	1530 MB	1530 MB	1
rollup-block-root-empty	1090 MB	1090 MB	1
rollup-block-root-single-tx	9410 MB	9410 MB	1
rollup-block-root	9420 MB	9420 MB	1
rollup-merge	344.77 MB	344.77 MB	1
rollup-root	354.75 MB	354.77 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: 846700b	Previous: 8e01c4a	Ratio
private-kernel-inner	202.35 MB	202.35 MB	1
private-kernel-reset	226.1 MB	226.1 MB	1
private-kernel-tail	177.64 MB	177.64 MB	1
rollup-base-private	507.46 MB	507.42 MB	1.00
rollup-base-public	440.61 MB	440.61 MB	1
rollup-block-root	1510 MB	1510 MB	1
rollup-merge	329.7 MB	329.7 MB	1
rollup-root	331.8 MB	331.8 MB	1
semaphore_depth_10	71.01 MB	71.01 MB	1

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

[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: 2066ca6	Previous: a77d56d	Ratio
semaphore-depth-10	0.028 s	0.02 s	1.40

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

CC: @TomAFrench

[TomAFrench]

nice!

[aakoshh]

Awesome! Could you remove DIE from here?

noir/compiler/noirc_evaluator/src/ssa.rs

Line 295 in 3c8f827

SsaPass::new(Ssa::dead_instruction_elimination, "Dead Instruction Elimination"),

[asterite]

@aakoshh Done! I also removed it from the main passes which was done for a similar reason.

There's still this comment which I think became outdated now but I don't know what should change:

        // The Brillig globals pass expected that we have the used globals map set for each function.
        // The used globals map is determined during DIE, so we should duplicate entry points before a DIE pass run.
        SsaPass::new(Ssa::brillig_entry_point_analysis, "Brillig Entry Point Analysis")
            // Remove any potentially unnecessary duplication from the Brillig entry point analysis.
            .and_then(Ssa::remove_unreachable_functions),

[asterite]

Hmmm... that change introduced regression. Maybe we can still run DIE but not because of used_globals?

[aakoshh]

@aakoshh Done! I also removed it from the main passes which was done for a similar reason.

There's still this comment which I think became outdated now but I don't know what should change:

        // The used globals map is determined during DIE, so we should duplicate entry points before a DIE pass run.
        SsaPass::new(Ssa::brillig_entry_point_analysis, "Brillig Entry Point Analysis")

As far as I understand that pass, it duplicates brillig functions so that the global addresses don't get mixed up.
Because of that, DIE had to run after this, to correctly determine which globals are used in which entry point.
With the removal of the used globals routine from DIE, it shouldn't matter which order they run in.

[aakoshh]

that change introduced regression.

Which change exactly? The removal of DIE from the minimal pipeline? What is the regression?

[asterite]

Which change exactly? The removal of DIE from the minimal pipeline? What is the regression?

I went ahead and remove one DIE from the main pipeline which was done for the same purpose... or so I thought! I shouldn't have done that 😅 Now it's undone, I think there shouldn't be any regressions.