feat: push overflow checks inside of signed binary ops#9074
Conversation
|
Code is relatively messy atm, I'll be doing a little bit of cleaning before marking as ready for review. |
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ 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: 4063332 | Previous: cf71daf | Ratio |
|---|---|---|---|
test_report_noir-lang_noir_bigcurve_ |
392 s |
263 s |
1.49 |
test_report_zkpassport_noir_rsa_ |
1 s |
0 s |
+∞ |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ 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: 65ad060 | Previous: cf71daf | Ratio |
|---|---|---|---|
rollup-block-root |
289 s |
197 s |
1.47 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ 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: 8302641 | Previous: d31ebb9 | Ratio |
|---|---|---|---|
rollup-root |
0.005 s |
0.004 s |
1.25 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
| /// something we take can advantage of in the [secondary_passes]. | ||
| pub fn primary_passes(options: &SsaEvaluatorOptions) -> Vec<SsaPass> { | ||
| vec![ | ||
| SsaPass::new(Ssa::expand_signed_checks, "expand signed checks"), |
There was a problem hiding this comment.
Having this at the very beginning of the SSA pipeline is intentional as I want to ensure that we replicate the current behaviour. In a followup PR we can push this further down so that we can ideally mark more signed operations as unchecked.
| /// Insert a numeric constant into the current function | ||
| fn numeric_constant(&mut self, value: impl Into<FieldElement>, typ: NumericType) -> ValueId { | ||
| self.context.dfg.make_constant(value.into(), typ) | ||
| } | ||
|
|
||
| /// Insert a binary instruction at the end of the current block. | ||
| /// Returns the result of the binary instruction. | ||
| fn insert_binary(&mut self, lhs: ValueId, operator: BinaryOp, rhs: ValueId) -> ValueId { | ||
| let instruction = Instruction::Binary(Binary { lhs, rhs, operator }); | ||
| self.context.insert_instruction(instruction, None).first() | ||
| } | ||
|
|
||
| /// Insert a not instruction at the end of the current block. | ||
| /// Returns the result of the instruction. | ||
| fn insert_not(&mut self, rhs: ValueId) -> ValueId { | ||
| self.context.insert_instruction(Instruction::Not(rhs), None).first() | ||
| } | ||
|
|
||
| /// Insert a truncate instruction at the end of the current block. | ||
| /// Returns the result of the truncate instruction. | ||
| fn insert_truncate(&mut self, value: ValueId, bit_size: u32, max_bit_size: u32) -> ValueId { | ||
| self.context | ||
| .insert_instruction(Instruction::Truncate { value, bit_size, max_bit_size }, None) | ||
| .first() | ||
| } | ||
|
|
||
| /// Insert a cast instruction at the end of the current block. | ||
| /// Returns the result of the cast instruction. | ||
| fn insert_cast(&mut self, value: ValueId, typ: NumericType) -> ValueId { | ||
| self.context.insert_instruction(Instruction::Cast(value, typ), None).first() | ||
| } | ||
|
|
||
| /// Insert a [`Instruction::RangeCheck`] instruction at the end of the current block. | ||
| fn insert_range_check( | ||
| &mut self, | ||
| value: ValueId, | ||
| max_bit_size: u32, | ||
| assert_message: Option<String>, | ||
| ) { | ||
| self.context.insert_instruction( | ||
| Instruction::RangeCheck { value, max_bit_size, assert_message }, | ||
| None, | ||
| ); | ||
| } | ||
|
|
||
| /// Insert a constrain instruction at the end of the current block. | ||
| fn insert_constrain( | ||
| &mut self, | ||
| lhs: ValueId, | ||
| rhs: ValueId, | ||
| assert_message: Option<ConstrainError>, | ||
| ) { | ||
| self.context.insert_instruction(Instruction::Constrain(lhs, rhs, assert_message), None); | ||
| } |
There was a problem hiding this comment.
These should perhaps be pushed onto the inner context as remove_bit_shifts has a lot of the same methods. Something for a followup however.
Co-authored-by: Akosh Farkash <aakoshh@gmail.com>
Co-authored-by: Akosh Farkash <aakoshh@gmail.com>
Co-authored-by: Akosh Farkash <aakoshh@gmail.com>
I'm going to punt this as it's forcing basically a complete rewrite of this test suite. |
aakoshh
left a comment
aakoshh
left a comment
There was a problem hiding this comment.
🤞
I had some timeouts with the rollup-block-base contracts recently. Managed to reel it back in the stack overflow PR I have, but there I had a sense of why it might have slowed down. Is there anything here that could explain a longer compilation time?
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
4 participants
Description
Problem*
Supercedes #9059
Resolves #9047
Summary*
This PR aims to put signed integer instruction in SSA on a similar footing to unsigned integer operations, i.e. unless explicitly marked as unchecked then it internally contains an appropriate overflow check in isolation.
I've basically just moved the relevant code from the ssa_codegen step and moved it into the new
expand_signed_checksssa pass. There's now a good deal of code duplication between this pass andremove_bit_shiftswhich implies we should push some of these methods into thesimple_optimizationmodule to be shared.I've removed the SSA validation around signed overflow checks (cc @vezenovm) as this is no longer necessary as we can rely on the testing of this new SSA pass to provide the same guarantees.
Additional Context
I've looked into the changes in
higher_order_functionsa little as I initially expected that this would not affect any of the build artifacts. It seems like the cause is that because the signed operation is marked as unchecked now, the LICM pass picks it up now which unlocks some extra optimization.Documentation*
Check one:
PR Checklist*
cargo fmton default settings.