chore(ssa): Simplify truncate when the bit size we are truncating to is greater than the value's max bit size

[vezenovm]

Description

Problem*

No issue just something I noticed when looking at the Truncate simplification.

Summary*

If we are truncating to a bit_size but the max_bit_size is less than that value, we can return the value.

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: 24a3512	Previous: 12bf0db	Ratio
purely_sequential_opcodes	252898 ns/iter (± 493)	254138 ns/iter (± 1529)	1.00
perfectly_parallel_opcodes	226063 ns/iter (± 12557)	225984 ns/iter (± 1376)	1.00
perfectly_parallel_batch_inversion_opcodes	2822222 ns/iter (± 8222)	2791262 ns/iter (± 10832)	1.01

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

[github-actions]

Artifact Size

Details

Benchmark suite	Current: 24a3512	Previous: 12bf0db	Ratio
private-kernel-inner	1134.4 KB	1134.4 KB	1
private-kernel-reset	2071.4 KB	2071.4 KB	1
private-kernel-tail	588.5 KB	588.5 KB	1
rollup-base-private	4944.6 KB	4944.6 KB	1
rollup-base-public	3986.2 KB	3986.2 KB	1
rollup-block-root-empty	240.3 KB	240.3 KB	1
rollup-block-root-single-tx	25691.2 KB	25691.2 KB	1
rollup-block-root	25717.7 KB	25717.7 KB	1
rollup-merge	184.7 KB	184.7 KB	1
rollup-root	420.8 KB	420.8 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: 24a3512	Previous: 12bf0db	Ratio
private-kernel-inner	0.027 s	0.027 s	1
private-kernel-reset	0.154 s	0.155 s	0.99
private-kernel-tail	0.011 s	0.011 s	1
rollup-base-private	0.29 s	0.293 s	0.99
rollup-base-public	0.184 s	0.191 s	0.96
rollup-block-root	10.6 s	10.7 s	0.99
rollup-merge	0.004 s	0.004 s	1
rollup-root	0.009 s	0.009 s	1
semaphore-depth-10	0.019 s	0.02 s	0.95

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

[github-actions]

Compilation Time

Details

Benchmark suite	Current: 24a3512	Previous: 12bf0db	Ratio
private-kernel-inner	2.374 s	2.354 s	1.01
private-kernel-reset	7.308 s	7.536 s	0.97
private-kernel-tail	1.124 s	1.108 s	1.01
rollup-base-private	15.96 s	16.14 s	0.99
rollup-base-public	13.04 s	13.28 s	0.98
rollup-block-root-empty	1.282 s	1.332 s	0.96
rollup-block-root-single-tx	133 s	127 s	1.05
rollup-block-root	122 s	127 s	0.96
rollup-merge	1.11 s	1.094 s	1.01
rollup-root	1.528 s	1.65 s	0.93
semaphore-depth-10	0.764 s	0.755 s	1.01

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

[github-actions]

Execution Memory

Details

Benchmark suite	Current: 24a3512	Previous: 12bf0db	Ratio
private-kernel-inner	202.09 MB	202.09 MB	1
private-kernel-reset	225.81 MB	225.81 MB	1
private-kernel-tail	177.35 MB	177.35 MB	1
rollup-base-private	489.89 MB	489.89 MB	1
rollup-base-public	423.19 MB	423.19 MB	1
rollup-block-root	1400 MB	1400 MB	1
rollup-merge	312.02 MB	312.02 MB	1
rollup-root	317.7 MB	317.7 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: 24a3512	Previous: 12bf0db	Ratio
test_report_AztecProtocol_aztec-packages_noir-projects_aztec-nr	70 s	70 s	1
test_report_AztecProtocol_aztec-packages_noir-projects_noir-contracts	111 s	110 s	1.01
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob	46 s	42 s	1.10
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_private-kernel-lib	214 s	203 s	1.05
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_rollup-lib	257 s	272 s	0.94
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_types	72 s	75 s	0.96
test_report_noir-lang_noir-bignum_	377 s	372 s	1.01
test_report_noir-lang_noir_bigcurve_	226 s	219 s	1.03
test_report_noir-lang_sha512_	30 s	28 s	1.07
test_report_zkpassport_noir_rsa_	1 s	1 s	1

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

[github-actions]

Compilation Memory

Details

Benchmark suite	Current: 24a3512	Previous: 12bf0db	Ratio
private-kernel-inner	292.92 MB	292.94 MB	1.00
private-kernel-reset	534.02 MB	534.02 MB	1
private-kernel-tail	191.54 MB	191.56 MB	1.00
rollup-base-private	1380 MB	1380 MB	1
rollup-base-public	1510 MB	1510 MB	1
rollup-block-root-empty	339.37 MB	339.39 MB	1.00
rollup-block-root-single-tx	7830 MB	7830 MB	1
rollup-block-root	7830 MB	7830 MB	1
rollup-merge	327.17 MB	327.17 MB	1
rollup-root	381.43 MB	381.44 MB	1.00
semaphore_depth_10	106.39 MB	106.39 MB	1

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

[TomAFrench]

I think that we should perhaps just add this as a validation check. I would not expect us to generate these instructions in the first place.

[jfecher]

I think it's fine to have this check. It seems obviously correct and I don't see a need to move it to a separate validation pass to enforce we never generate instructions like this

[asterite]

Maybe add a small test? It could be done using Ssa::from_str_strimplifying

[TomAFrench]

It seems obviously correct

I agree that the simplification is correct but the fact that we would generate such a case implies that something is not behaving as we'd expect which warrants investigation.

[jfecher]

but the fact that we would generate such a case implies that something is not behaving as we'd expect which warrants investigation.

I could see us in the future adding a pass to calculate e.g. smaller max sizes for truncate instructions by trying to thread through more information (e.g. two u8s which were cast to u32 and added, then the resulting u32 truncated to u16 should actually be a free case) so I don't think us generating it would necessarily be indicative of something concerning unless we're generating incorrect max bit widths.

[vezenovm]

The only place I think where this may be possible at the moment is in remove_bit_shifts (although I don't see any performance improvements except the compilation time for rollup-base-public). Take a look at this truncate

noir/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs

Line 151 in 12bf0db

let result = self.insert_truncate(result, bit_size, max_bit);

The max_bit_size can come from DataFlowGraph::get_value_max_num_bits which I believe could return a max_bit_size less than the bit size of the type itself. I don't think us having a max_bit_size less than the bit_size we are truncating to is inherently bad SSA form. Especially as we add more value range analysis logic in the future.

I have added a small test as well using Ssa::from_str_strimplifying that would have previously not simplified.

[vezenovm]

To Tom's point, the test I wrote would not be generated by our initial SSA gen. We have this match case in insert_safe_cast:

            (
                Type::Numeric(NumericType::Unsigned { bit_size: incoming_type_size }),
                NumericType::Unsigned { bit_size: target_type_size },
            ) => {
                // If target size is smaller, we do a truncation
                if target_type_size < *incoming_type_size {
                    value =
                        self.builder.insert_truncate(value, target_type_size, *incoming_type_size);
                }
                value
            }

However, with this simplification we would not need to special case whether we insert a truncate in the initial SSA gen and leave it entirely to our instruction simplifications, which I view as a better separation of concerns. For now, I have left SSA gen untouched though. I am going to merge as I don't think this simplification implies ill formed SSA.