Skip to content

fix(ssa): Validate signed mul overflow#8798

Merged
vezenovm merged 8 commits intomv/validation-modulefrom
mv/validate-signed-mul-overflow
Jun 5, 2025
Merged

fix(ssa): Validate signed mul overflow#8798
vezenovm merged 8 commits intomv/validation-modulefrom
mv/validate-signed-mul-overflow

Conversation

@vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Jun 4, 2025

Description

Problem*

Resolves #8088

Builds upon #8765.

Summary*

This adds validation similar to #8706, except for signed multiplication. The semantics for which are seen here. We could be more strict with these checks by checking this code gen as well:

self.check_signed_overflow(result, lhs, rhs, operator, bit_size, location);
self.insert_safe_cast(result, result_type, location)

However, for now I have chosen to simply check up to the truncate point to simplify our "state machine". We also mainly want to guarantee that we have truncated the result of an operation appropriately so that someone cannot code gen an overflow.

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.

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 'ACVM Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: d1dd840 Previous: 9d44ab7 Ratio
perfectly_parallel_batch_inversion_opcodes 3564172 ns/iter (± 1987) 2778522 ns/iter (± 7503) 1.28

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 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: a9d8f3c Previous: 9d44ab7 Ratio
test_report_zkpassport_noir_rsa_ 3 s 2 s 1.50

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: fd50edb Previous: c2cedd4 Ratio
rollup-merge 0.004 s 0.003 s 1.33

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

CC: @TomAFrench

Comment on lines -561 to -563
for function in ssa.functions.values() {
validate_function(function);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this gone? (maybe it's already done somewhere else and this was redundant?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was redundant. It was being done in FunctionBuilder::finish

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.

Looks good! Would a test for mul followed by truncate but not by any cast be worth having?

@asterite
Copy link
Collaborator

asterite commented Jun 5, 2025

Looks good?

Argh, I meant to type "!" instead of "?" 😅

@vezenovm
Copy link
Contributor Author

vezenovm commented Jun 5, 2025

Would a test for mul followed by truncate but not by any cast be worth having?

Yes, I will add one.

@vezenovm
Copy link
Contributor Author

vezenovm commented Jun 5, 2025

Added a test for a mul followed by a truncate with no cast. I also added a test that a lone truncate compiles:

        acir(inline) pure fn main f0 {
          b0(v0: i16):
            v1 = truncate v0 to 8 bits, max_bit_size: 8
            return v1
        }

@vezenovm vezenovm merged commit bf459fd into mv/validation-module Jun 5, 2025
28 of 29 checks passed
@vezenovm vezenovm deleted the mv/validate-signed-mul-overflow branch June 5, 2025 19:20
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.

2 participants