feat: simplify simple conditionals for brillig #7205
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
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
| 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?
There was a problem hiding this comment.
No, if the two mapping both map to the same value, I think that's fine.
However, if the value of one is the key of the other, then one mapping is somehow overwriting what the other mapping is doing, and I disallow this. Especially because mappings are constructed in reverse order, so a mapping overwriting the 'next one' is probably bad.
| 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 added a warning instead of a todo at the beginning of the function to indicate that the numbers are estimates and can be improved.
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. |
I am not sure where these numbers come exactly, doing nargo info on brillig_conditional, I get 31 opcodes with the simplification VS 32 without: brillig_conditional | conditional | N/A | N/A | 32 | |
|
The regression is in the number of opcodes executed (i.e. the execution trace length). This can be shown by |
Oh right, then this is expected, since the optimisation execute both branches, it is expected to get more opcode executed, it's just that we avoid jumping around. For a number of cases, the numbers are better because of handling of arrays |
| | Instruction::EnableSideEffectsIf { .. } | ||
| | Instruction::IncrementRc { .. } | ||
| | Instruction::DecrementRc { .. } | ||
| | Instruction::MakeArray { .. } |
There was a problem hiding this comment.
We want to get a more accurate instruction count here. MakeArray is either several store and add instructions or a runtime loop for large non-nested arrays
.There was a problem hiding this comment.
I put 20 as the cost then, since it seems to be the maximum cost (10 stores and ~10 index increments)
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
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: 88faf2c | Previous: 819a53a | Ratio |
|---|---|---|---|
private-kernel-inner |
2.404 s |
1.958 s |
1.23 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
| source_a: MemoryAddress, | ||
| source_b: MemoryAddress, | ||
| condition: MemoryAddress, | ||
| ) { | ||
| debug_println!( | ||
| self.enable_debug_trace, | ||
| " {} = CONDITIONAL MOV {} then {}, else {}", | ||
| destination, | ||
| condition, | ||
| source_a, | ||
| source_b |
There was a problem hiding this comment.
| source_a: MemoryAddress, | |
| source_b: MemoryAddress, | |
| condition: MemoryAddress, | |
| ) { | |
| debug_println!( | |
| self.enable_debug_trace, | |
| " {} = CONDITIONAL MOV {} then {}, else {}", | |
| destination, | |
| condition, | |
| source_a, | |
| source_b | |
| source_then: MemoryAddress, | |
| source_else: MemoryAddress, | |
| condition: MemoryAddress, | |
| ) { | |
| debug_println!( | |
| self.enable_debug_trace, | |
| " {} = CONDITIONAL MOV {} then {}, else {}", | |
| destination, | |
| condition, | |
| source_then, | |
| source_else |
I'd consider putting an if in there as well. Perhaps MOV_IF like JUMP_IF?
| @@ -213,6 +216,10 @@ struct Context<'f> { | |||
| /// us from unnecessarily inserting extra instructions, and keeps ids unique which | |||
| /// helps simplifications. | |||
| not_instructions: HashMap<ValueId, ValueId>, | |||
|
|
|||
| /// Flag to tell the context to not issue 'enable_side_effect' instructions during flattening. | |||
| /// This should set to true only by flatten_single(), when none instruction is known to fail. | |||
There was a problem hiding this comment.
| /// This should set to true only by flatten_single(), when none instruction is known to fail. | |
| /// This should be set to true only by `flatten_single()`, when no instruction is known to fail. |
|
|
||
| impl Ssa { | ||
| #[tracing::instrument(level = "trace", skip(self))] | ||
| pub(crate) fn flatten_basic_conditionals(mut self) -> Ssa { |
There was a problem hiding this comment.
It would be helpful to add a doctoring with a basic example of what this pass achieves. For example when we walk someone through the passes, it's a quick way to explain what we mean without having to look up unit tests. Maybe examples are not realistic in more complex cases, and having them here but not there is just irregular, but at sentence or two of what it's looking for and what tradeoffs it's considering would be great IMO.
| // Returns the blocks of the simple conditional sub-graph whose input block is the entry. | ||
| // Returns None if the input block is not the entry block of a simple conditional. |
There was a problem hiding this comment.
| // Returns the blocks of the simple conditional sub-graph whose input block is the entry. | |
| // Returns None if the input block is not the entry block of a simple conditional. | |
| /// Returns the blocks of the simple conditional sub-graph whose input block is the entry. | |
| /// Returns None if the input block is not the entry block of a simple conditional. |
| } | ||
| } | ||
| } else if right_successors_len == 1 && next_right == Some(left) { | ||
| // Right branch joins the right branch, it is a if/else statement with no then |
There was a problem hiding this comment.
| // Right branch joins the right branch, it is a if/else statement with no then | |
| // Right branch joins the left branch, it is a if/else statement with no then |
| let (block_then, block_else) = if left == *then_destination { | ||
| (Some(left), None) | ||
| } else if left == *else_destination { | ||
| (None, Some(left)) |
There was a problem hiding this comment.
It's a bit confusing that the comment above says "it is a if/then statement with no else" but then here we can have an else and no then. (I'm not sure what that means tbh).
| //0. initialize the context for flattening a 'single conditional' | ||
| let mut queue = vec![]; | ||
| self.target_block = conditional.block_entry; | ||
| self.no_predicate = true; |
There was a problem hiding this comment.
I don't see this being reset anywhere. Ostensibly the Context could outlive the call to flatten_single_conditional, in which case this should go back to where it was before the call, no?
| // Manually set the terminator of the entry block to the one of the exit block | ||
| let terminator = | ||
| self.inserter.function.dfg[conditional.block_exit].terminator().unwrap().clone(); | ||
| let mut next_blocks = VecDeque::new(); |
There was a problem hiding this comment.
I may be blind, but where is this consumed?
| if !queue.contains(&incoming_block) { | ||
| queue.push(incoming_block); |
There was a problem hiding this comment.
I don't see see these values being popped: the 3rd section handles the exit block, and there is no looping 👀
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.