From 7f64ea5da2fa6edfa0bec9ca3d11a863a52dafa1 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 22 Sep 2023 11:21:20 -0500 Subject: [PATCH 1/3] Optimize out constant ifs in flattening pass --- .../src/ssa/opt/flatten_cfg.rs | 118 +++++++++++++----- 1 file changed, 89 insertions(+), 29 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index d57c2cc7933..2b2c803a7af 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -286,35 +286,23 @@ impl<'f> Context<'f> { let else_block = *else_destination; let then_condition = self.inserter.resolve(old_condition); - let one = FieldElement::one(); - let then_branch = - self.inline_branch(block, then_block, old_condition, then_condition, one); - - let else_condition = - self.insert_instruction(Instruction::Not(then_condition), CallStack::new()); - let zero = FieldElement::zero(); - - // Make sure the else branch sees the previous values of each store - // rather than any values created in the 'then' branch. - self.undo_stores_in_then_branch(&then_branch); - - let else_branch = - self.inline_branch(block, else_block, old_condition, else_condition, zero); - - // We must remember to reset whether side effects are enabled when both branches - // end, in addition to resetting the value of old_condition since it is set to - // known to be true/false within the then/else branch respectively. - self.insert_current_side_effects_enabled(); - - // We must map back to `then_condition` here. Mapping `old_condition` to itself would - // lose any previous mappings. - self.inserter.map_value(old_condition, then_condition); - - // While there is a condition on the stack we don't compile outside the condition - // until it is popped. This ensures we inline the full then and else branches - // before continuing from the end of the conditional here where they can be merged properly. - let end = self.branch_ends[&block]; - self.inline_branch_end(end, then_branch, else_branch) + if let Some(condition) = + self.inserter.function.dfg.get_numeric_constant(then_condition) + { + if condition.is_zero() { + self.handle_constant_jmpif(block, else_block, old_condition) + } else { + self.handle_constant_jmpif(block, then_block, old_condition) + } + } else { + self.handle_non_constant_jmpif( + block, + then_block, + else_block, + old_condition, + then_condition, + ) + } } TerminatorInstruction::Jmp { destination, arguments, call_stack: _ } => { if let Some((end_block, _)) = self.conditions.last() { @@ -337,6 +325,78 @@ impl<'f> Context<'f> { } } + fn handle_constant_jmpif( + &mut self, + block: BasicBlockId, + branch_block: BasicBlockId, + old_condition: ValueId, + ) -> BasicBlockId { + // This value will be unused since the condition is constant + let one = FieldElement::one(); + let new_condition = self.inserter.function.dfg.make_constant(one, Type::bool()); + + let branch = self.inline_branch(block, branch_block, old_condition, new_condition, one); + + // We must remember to reset whether side effects are enabled when both branches + // end, in addition to resetting the value of old_condition since it is set to + // known to be true/false within the then/else branch respectively. + self.insert_current_side_effects_enabled(); + + // We must map back to `new_condition` here. Mapping `old_condition` to itself would + // lose any previous mappings. + self.inserter.map_value(old_condition, new_condition); + + // While there is a condition on the stack we don't compile outside the condition + // until it is popped. This ensures we inline the full then and else branches + // before continuing from the end of the conditional here where they can be merged properly. + let end = self.branch_ends[&block]; + + let args = self.inserter.function.dfg[branch.last_block].terminator_arguments().to_vec(); + let params = self.inserter.function.dfg.block_parameters(end); + assert_eq!(params.len(), args.len()); + + // insert merge instruction + self.inline_block(end, &args) + } + + fn handle_non_constant_jmpif( + &mut self, + block: BasicBlockId, + then_block: BasicBlockId, + else_block: BasicBlockId, + old_condition: ValueId, + then_condition: ValueId, + ) -> BasicBlockId { + let one = FieldElement::one(); + let then_branch = self.inline_branch(block, then_block, old_condition, then_condition, one); + + let else_condition = + self.insert_instruction(Instruction::Not(then_condition), CallStack::new()); + let zero = FieldElement::zero(); + + // Make sure the else branch sees the previous values of each store + // rather than any values created in the 'then' branch. + self.undo_stores_in_then_branch(&then_branch); + + let else_branch = + self.inline_branch(block, else_block, old_condition, else_condition, zero); + + // We must remember to reset whether side effects are enabled when both branches + // end, in addition to resetting the value of old_condition since it is set to + // known to be true/false within the then/else branch respectively. + self.insert_current_side_effects_enabled(); + + // We must map back to `then_condition` here. Mapping `old_condition` to itself would + // lose any previous mappings. + self.inserter.map_value(old_condition, then_condition); + + // While there is a condition on the stack we don't compile outside the condition + // until it is popped. This ensures we inline the full then and else branches + // before continuing from the end of the conditional here where they can be merged properly. + let end = self.branch_ends[&block]; + self.inline_branch_end(end, then_branch, else_branch) + } + /// Push a condition to the stack of conditions. /// /// This condition should be present while we're inlining each block reachable from the 'then' From d24f9c0fa1f364a5a5233d0c617971cec453623c Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 22 Sep 2023 11:48:55 -0500 Subject: [PATCH 2/3] Revert "Optimize out constant ifs in flattening pass" This reverts commit 7f64ea5da2fa6edfa0bec9ca3d11a863a52dafa1. --- .../src/ssa/opt/flatten_cfg.rs | 118 +++++------------- 1 file changed, 29 insertions(+), 89 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 2b2c803a7af..d57c2cc7933 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -286,23 +286,35 @@ impl<'f> Context<'f> { let else_block = *else_destination; let then_condition = self.inserter.resolve(old_condition); - if let Some(condition) = - self.inserter.function.dfg.get_numeric_constant(then_condition) - { - if condition.is_zero() { - self.handle_constant_jmpif(block, else_block, old_condition) - } else { - self.handle_constant_jmpif(block, then_block, old_condition) - } - } else { - self.handle_non_constant_jmpif( - block, - then_block, - else_block, - old_condition, - then_condition, - ) - } + let one = FieldElement::one(); + let then_branch = + self.inline_branch(block, then_block, old_condition, then_condition, one); + + let else_condition = + self.insert_instruction(Instruction::Not(then_condition), CallStack::new()); + let zero = FieldElement::zero(); + + // Make sure the else branch sees the previous values of each store + // rather than any values created in the 'then' branch. + self.undo_stores_in_then_branch(&then_branch); + + let else_branch = + self.inline_branch(block, else_block, old_condition, else_condition, zero); + + // We must remember to reset whether side effects are enabled when both branches + // end, in addition to resetting the value of old_condition since it is set to + // known to be true/false within the then/else branch respectively. + self.insert_current_side_effects_enabled(); + + // We must map back to `then_condition` here. Mapping `old_condition` to itself would + // lose any previous mappings. + self.inserter.map_value(old_condition, then_condition); + + // While there is a condition on the stack we don't compile outside the condition + // until it is popped. This ensures we inline the full then and else branches + // before continuing from the end of the conditional here where they can be merged properly. + let end = self.branch_ends[&block]; + self.inline_branch_end(end, then_branch, else_branch) } TerminatorInstruction::Jmp { destination, arguments, call_stack: _ } => { if let Some((end_block, _)) = self.conditions.last() { @@ -325,78 +337,6 @@ impl<'f> Context<'f> { } } - fn handle_constant_jmpif( - &mut self, - block: BasicBlockId, - branch_block: BasicBlockId, - old_condition: ValueId, - ) -> BasicBlockId { - // This value will be unused since the condition is constant - let one = FieldElement::one(); - let new_condition = self.inserter.function.dfg.make_constant(one, Type::bool()); - - let branch = self.inline_branch(block, branch_block, old_condition, new_condition, one); - - // We must remember to reset whether side effects are enabled when both branches - // end, in addition to resetting the value of old_condition since it is set to - // known to be true/false within the then/else branch respectively. - self.insert_current_side_effects_enabled(); - - // We must map back to `new_condition` here. Mapping `old_condition` to itself would - // lose any previous mappings. - self.inserter.map_value(old_condition, new_condition); - - // While there is a condition on the stack we don't compile outside the condition - // until it is popped. This ensures we inline the full then and else branches - // before continuing from the end of the conditional here where they can be merged properly. - let end = self.branch_ends[&block]; - - let args = self.inserter.function.dfg[branch.last_block].terminator_arguments().to_vec(); - let params = self.inserter.function.dfg.block_parameters(end); - assert_eq!(params.len(), args.len()); - - // insert merge instruction - self.inline_block(end, &args) - } - - fn handle_non_constant_jmpif( - &mut self, - block: BasicBlockId, - then_block: BasicBlockId, - else_block: BasicBlockId, - old_condition: ValueId, - then_condition: ValueId, - ) -> BasicBlockId { - let one = FieldElement::one(); - let then_branch = self.inline_branch(block, then_block, old_condition, then_condition, one); - - let else_condition = - self.insert_instruction(Instruction::Not(then_condition), CallStack::new()); - let zero = FieldElement::zero(); - - // Make sure the else branch sees the previous values of each store - // rather than any values created in the 'then' branch. - self.undo_stores_in_then_branch(&then_branch); - - let else_branch = - self.inline_branch(block, else_block, old_condition, else_condition, zero); - - // We must remember to reset whether side effects are enabled when both branches - // end, in addition to resetting the value of old_condition since it is set to - // known to be true/false within the then/else branch respectively. - self.insert_current_side_effects_enabled(); - - // We must map back to `then_condition` here. Mapping `old_condition` to itself would - // lose any previous mappings. - self.inserter.map_value(old_condition, then_condition); - - // While there is a condition on the stack we don't compile outside the condition - // until it is popped. This ensures we inline the full then and else branches - // before continuing from the end of the conditional here where they can be merged properly. - let end = self.branch_ends[&block]; - self.inline_branch_end(end, then_branch, else_branch) - } - /// Push a condition to the stack of conditions. /// /// This condition should be present while we're inlining each block reachable from the 'then' From 2cd7e6f447430954f37f28bd1c2abf30dffabd2e Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 22 Sep 2023 11:54:14 -0500 Subject: [PATCH 3/3] Fix subtract with underflow --- compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index d57c2cc7933..7700c7051ab 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -461,7 +461,7 @@ impl<'f> Context<'f> { let mut get_element = |array, typevars, len| { // The smaller slice is filled with placeholder data. Codegen for slice accesses must // include checks against the dynamic slice length so that this placeholder data is not incorrectly accessed. - if (len - 1) < index_value.to_u128() as usize { + if len <= index_value.to_u128() as usize { let zero = FieldElement::zero(); self.inserter.function.dfg.make_constant(zero, Type::field()) } else {