feat: simplify simple conditionals for brillig #7205
Conversation
![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 'Execution Memory'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 56ef385 | Previous: 8d39337 | Ratio |
|---|---|---|---|
rollup-block-root |
4900 MB |
1230 MB |
3.98 |
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 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: b38e806 | Previous: d5d6cb7 | Ratio |
|---|---|---|---|
noir-lang_schnorr_ |
1 s |
0 s |
+∞ |
noir-lang_noir_string_search_ |
1 s |
0 s |
+∞ |
noir-lang_noir_base64_ |
2 s |
1 s |
2 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
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: 06d26db | Previous: a9e9850 | Ratio |
|---|---|---|---|
global_var_regression_entry_points |
0.009 s |
0.007 s |
1.29 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
| BrilligVariable::BrilligArray(else_array), | ||
| ) => { | ||
| // Pointer to the array which result from the if-else | ||
| let pointer = self.brillig_context.allocate_register(); |
There was a problem hiding this comment.
Do we need to allocate another register here? Can we just use the result from above and result.extract_register()?
| let mut instruction = self.function.dfg[id].clone(); | ||
| instruction.map_values_mut(|id| self.resolve(id)); | ||
| self.function.dfg.set_instruction(id, instruction); | ||
| } |
There was a problem hiding this comment.
| } | |
| } | |
nit just to separate the } from the next comment
| if mapping.contains_key(k) { | ||
| unreachable!("cannot merge key"); | ||
| } | ||
| if mapping.contains_key(v) { |
There was a problem hiding this comment.
We want to check contains_key for the value here?
| BinaryOp::Lt => 5, | ||
| BinaryOp::And | ||
| | BinaryOp::Or | ||
| | BinaryOp::Xor => 1, //todo |
There was a problem hiding this comment.
This TODO doesn't have a description and there are a couple TODOs in this function. Could you make issues to handle them if we are not going to in this PR?
There was a problem hiding this comment.
Happy to address these in this PR. The point is to provide realistic brillig cost for each opcode. The idea is to improve execution speed so the cost should be the runtime cost in whatever unit as along as it is the same for all opcodes. My numbers can certainly be improved.
I will add a warning instead of a todo at the beginning of the function to indicate that the numbers are estimates and can be improved.
| let index = dfg.get_numeric_constant(*index); | ||
| let mut ok_bound = false; | ||
| if let Some(index) = index { |
There was a problem hiding this comment.
| let index = dfg.get_numeric_constant(*index); | |
| let mut ok_bound = false; | |
| if let Some(index) = index { | |
| let mut ok_bound = false; | |
| if let (Some(index), Some(len)) = (dfg.get_numeric_constant(*index), dfg.try_get_array_length(*array)) { |
I think something like this would be more idiomatic and reduces nesting
| let po = post_order.as_slice(); | ||
| for block in po { |
There was a problem hiding this comment.
| let po = post_order.as_slice(); | |
| for block in po { | |
| for block in post_order.as_slice() { |
| function: &Function, | ||
| ) -> Option<BasicConditional> { | ||
| // jump overhead is the cost for doing the conditional and jump around the blocks | ||
| // I use 10 as a rough estimate, real cost is less. |
There was a problem hiding this comment.
| // I use 10 as a rough estimate, real cost is less. | |
| // We use 10 as a rough estimate, the real cost is less. |
| let mut successors = cfg.successors(block); | ||
| let mut result = None; | ||
| // a conditional must have 2 branches | ||
| if successors.len() == 2 { |
There was a problem hiding this comment.
Let's just early return with None if successors.len() != 2 to reduce this nesting
| call_stack: _, | ||
| }) = function.dfg[block].terminator() | ||
| { | ||
| if left == *then_destination { |
There was a problem hiding this comment.
I think this would be more readable if we did something like:
let (block_then, block_else) = if left == *then_destination {
(Some(left), None)
} else if left == *else_destination {
(None, Some(left))
};
result = Some(BasicConditional {
block_entry: block,
block_then,
block_else,
block_exit: right,
});| call_stack: _, | ||
| }) = function.dfg[block].terminator() | ||
| { | ||
| if right == *else_destination { |
There was a problem hiding this comment.
Same thing as above for the else if left_successors_len == 1 && next_left == Some(right) { case
Agreed that it would be good to have more clarity on this. We've got some serious benefits when conditionally mutating large structs but there's quite a few regressions. |
Description
Problem*
Resolves #6394
Summary*
Add a pass which simplify simple if statements in brillig functions
Additional Context
The PR is working fine now, the memory consumption alert does not seem to be up-to-date.
The report from the last commit is correct: https://github.com/noir-lang/noir/actions/runs/13119725684/artifacts/2528887358
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.