From 96b106802e7b6ea035a2f4a92ab0edd4767e7e44 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 3 Sep 2024 00:21:08 +0100 Subject: [PATCH 1/6] feat: simplify double jmps during `simplify_cfg` --- .../src/ssa/opt/simplify_cfg.rs | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index 0a3b2a800ca..d8d29dc2f2b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -52,6 +52,8 @@ fn simplify_function(function: &mut Function) { stack.extend(function.dfg[block].successors().filter(|block| !visited.contains(block))); } + check_for_double_jmp(function, block, &mut cfg); + // This call is before try_inline_into_predecessor so that if it succeeds in changing a // jmpif into a jmp, the block may then be inlined entirely into its predecessor in try_inline_into_predecessor. check_for_constant_jmpif(function, block, &mut cfg); @@ -102,6 +104,69 @@ fn check_for_constant_jmpif( } } +/// Optimize a jmp to a block which immediately jmps elsewhere to just jmp to the second block. +fn check_for_double_jmp(function: &mut Function, block: BasicBlockId, cfg: &mut ControlFlowGraph) { + if !function.dfg[block].instructions().is_empty() + || !function.dfg[block].parameters().is_empty() + { + return; + } + + if let Some(TerminatorInstruction::Jmp { destination: final_destination, arguments, .. }) = + function.dfg[block].terminator() + { + let final_destination = *final_destination; + + if !arguments.is_empty() { + return; + } + + let predecessors: Vec<_> = cfg.predecessors(block).collect(); + for predecessor_block in predecessors { + let terminator_instruction = function.dfg[predecessor_block].take_terminator(); + let redirected_terminator_instruction = match terminator_instruction { + TerminatorInstruction::JmpIf { + condition, + then_destination, + else_destination, + call_stack, + } => TerminatorInstruction::JmpIf { + condition, + then_destination: if then_destination == block { + final_destination + } else { + then_destination + }, + else_destination: if else_destination == block { + final_destination + } else { + else_destination + }, + call_stack, + }, + TerminatorInstruction::Jmp { destination, arguments, call_stack } => { + assert_eq!( + destination, block, + "ICE: predecessor block doesn't jump to current block" + ); + assert!(arguments.is_empty(), "ICE: predecessor jmp has arguments"); + TerminatorInstruction::Jmp { + destination: final_destination, + arguments, + call_stack, + } + } + TerminatorInstruction::Return { .. } => unreachable!( + "ICE: predecessor block should not have return terminator instruction" + ), + }; + + function.dfg[predecessor_block].set_terminator(redirected_terminator_instruction); + cfg.recompute_block(function, predecessor_block); + } + } +} + /// If the given block has block parameters, replace them with the jump arguments from the predecessor. /// /// Currently, if this function is needed, `try_inline_into_predecessor` will also always apply, From 28a57877be7d44c459a733d871619b6d02b70046 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 3 Sep 2024 10:33:40 +0100 Subject: [PATCH 2/6] . --- compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index d8d29dc2f2b..975d8cb51e9 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -52,8 +52,6 @@ fn simplify_function(function: &mut Function) { stack.extend(function.dfg[block].successors().filter(|block| !visited.contains(block))); } - check_for_double_jmp(function, block, &mut cfg); - // This call is before try_inline_into_predecessor so that if it succeeds in changing a // jmpif into a jmp, the block may then be inlined entirely into its predecessor in try_inline_into_predecessor. check_for_constant_jmpif(function, block, &mut cfg); @@ -74,6 +72,10 @@ fn simplify_function(function: &mut Function) { // optimizations performed after this point on the same block should check if // the inlining here was successful before continuing. try_inline_into_predecessor(function, &mut cfg, block, predecessor); + } else { + drop(predecessors); + + check_for_double_jmp(function, block, &mut cfg); } } } @@ -164,6 +166,7 @@ fn check_for_double_jmp(function: &mut Function, block: BasicBlockId, cfg: &mut function.dfg[predecessor_block].set_terminator(redirected_terminator_instruction); cfg.recompute_block(function, predecessor_block); } + cfg.recompute_block(function, block); } } From 928dec953914d4934ea2aac2773bad8e7769cf78 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 3 Sep 2024 10:48:18 +0100 Subject: [PATCH 3/6] . --- .../src/ssa/opt/simplify_cfg.rs | 22 +++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index 975d8cb51e9..1a386e46d22 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -30,7 +30,7 @@ impl Ssa { /// 5. Replacing any jmpifs with constant conditions with jmps. If this causes the block to have /// only 1 successor then (2) also will be applied. /// - /// Currently, 1 and 4 are unimplemented. + /// Currently, 1 is unimplemented. #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn simplify_cfg(mut self) -> Self { for function in self.functions.values_mut() { @@ -132,20 +132,24 @@ fn check_for_double_jmp(function: &mut Function, block: BasicBlockId, cfg: &mut then_destination, else_destination, call_stack, - } => TerminatorInstruction::JmpIf { - condition, - then_destination: if then_destination == block { + } => { + let then_destination = if then_destination == block { final_destination } else { then_destination - }, - else_destination: if else_destination == block { + }; + let else_destination = if else_destination == block { final_destination } else { else_destination - }, - call_stack, - }, + }; + TerminatorInstruction::JmpIf { + condition, + then_destination, + else_destination, + call_stack, + } + } TerminatorInstruction::Jmp { destination, arguments, call_stack } => { assert_eq!( destination, block, From 4c82e60ed83b24a52d84ba6f3d460ddffa1ad27b Mon Sep 17 00:00:00 2001 From: TomAFrench Date: Wed, 4 Sep 2024 12:51:07 +0000 Subject: [PATCH 4/6] . --- compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index 1a386e46d22..6327d614ee2 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -15,7 +15,7 @@ use acvm::acir::AcirField; use crate::ssa::{ ir::{ - basic_block::BasicBlockId, cfg::ControlFlowGraph, function::Function, + basic_block::BasicBlockId, cfg::ControlFlowGraph, function::{Function, RuntimeType}, instruction::TerminatorInstruction, }, ssa_gen::Ssa, @@ -74,7 +74,7 @@ fn simplify_function(function: &mut Function) { try_inline_into_predecessor(function, &mut cfg, block, predecessor); } else { drop(predecessors); - + check_for_double_jmp(function, block, &mut cfg); } } @@ -108,6 +108,11 @@ fn check_for_constant_jmpif( /// Optimize a jmp to a block which immediately jmps elsewhere to just jmp to the second block. fn check_for_double_jmp(function: &mut Function, block: BasicBlockId, cfg: &mut ControlFlowGraph) { + if matches!(function.runtime(), RuntimeType::Acir(_)) { + // We can't remove double jumps in ACIR functions as this interferes with the `flatten_cfg` pass. + return + } + if !function.dfg[block].instructions().is_empty() || !function.dfg[block].parameters().is_empty() { From fbc85f5b83ac24e5525684740e33b5629cdd4328 Mon Sep 17 00:00:00 2001 From: Tom French Date: Thu, 5 Sep 2024 13:40:54 +0100 Subject: [PATCH 5/6] . --- compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index 6327d614ee2..f1293814a74 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -15,7 +15,9 @@ use acvm::acir::AcirField; use crate::ssa::{ ir::{ - basic_block::BasicBlockId, cfg::ControlFlowGraph, function::{Function, RuntimeType}, + basic_block::BasicBlockId, + cfg::ControlFlowGraph, + function::{Function, RuntimeType}, instruction::TerminatorInstruction, }, ssa_gen::Ssa, @@ -74,7 +76,7 @@ fn simplify_function(function: &mut Function) { try_inline_into_predecessor(function, &mut cfg, block, predecessor); } else { drop(predecessors); - + check_for_double_jmp(function, block, &mut cfg); } } @@ -110,9 +112,9 @@ fn check_for_constant_jmpif( fn check_for_double_jmp(function: &mut Function, block: BasicBlockId, cfg: &mut ControlFlowGraph) { if matches!(function.runtime(), RuntimeType::Acir(_)) { // We can't remove double jumps in ACIR functions as this interferes with the `flatten_cfg` pass. - return + return; } - + if !function.dfg[block].instructions().is_empty() || !function.dfg[block].parameters().is_empty() { From 35bb09c78c71249b4d57047577c6b022f3a86864 Mon Sep 17 00:00:00 2001 From: Tom French Date: Thu, 5 Sep 2024 15:31:01 +0100 Subject: [PATCH 6/6] chore: switch to an early return --- .../src/ssa/opt/simplify_cfg.rs | 86 +++++++++---------- 1 file changed, 39 insertions(+), 47 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index f1293814a74..6887873dbc3 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -121,64 +121,56 @@ fn check_for_double_jmp(function: &mut Function, block: BasicBlockId, cfg: &mut return; } - if let Some(TerminatorInstruction::Jmp { destination: final_destination, arguments, .. }) = + let Some(TerminatorInstruction::Jmp { destination: final_destination, arguments, .. }) = function.dfg[block].terminator() - { - let final_destination = *final_destination; + else { + return; + }; - if !arguments.is_empty() { - return; - } + if !arguments.is_empty() { + return; + } - let predecessors: Vec<_> = cfg.predecessors(block).collect(); - for predecessor_block in predecessors { - let terminator_instruction = function.dfg[predecessor_block].take_terminator(); - let redirected_terminator_instruction = match terminator_instruction { + let final_destination = *final_destination; + + let predecessors: Vec<_> = cfg.predecessors(block).collect(); + for predecessor_block in predecessors { + let terminator_instruction = function.dfg[predecessor_block].take_terminator(); + let redirected_terminator_instruction = match terminator_instruction { + TerminatorInstruction::JmpIf { + condition, + then_destination, + else_destination, + call_stack, + } => { + let then_destination = + if then_destination == block { final_destination } else { then_destination }; + let else_destination = + if else_destination == block { final_destination } else { else_destination }; TerminatorInstruction::JmpIf { condition, then_destination, else_destination, call_stack, - } => { - let then_destination = if then_destination == block { - final_destination - } else { - then_destination - }; - let else_destination = if else_destination == block { - final_destination - } else { - else_destination - }; - TerminatorInstruction::JmpIf { - condition, - then_destination, - else_destination, - call_stack, - } - } - TerminatorInstruction::Jmp { destination, arguments, call_stack } => { - assert_eq!( - destination, block, - "ICE: predecessor block doesn't jump to current block" - ); - assert!(arguments.is_empty(), "ICE: predecessor jmp has arguments"); - TerminatorInstruction::Jmp { - destination: final_destination, - arguments, - call_stack, - } } - TerminatorInstruction::Return { .. } => unreachable!( - "ICE: predecessor block should not have return terminator instruction" - ), - }; + } + TerminatorInstruction::Jmp { destination, arguments, call_stack } => { + assert_eq!( + destination, block, + "ICE: predecessor block doesn't jump to current block" + ); + assert!(arguments.is_empty(), "ICE: predecessor jmp has arguments"); + TerminatorInstruction::Jmp { destination: final_destination, arguments, call_stack } + } + TerminatorInstruction::Return { .. } => { + unreachable!("ICE: predecessor block should not have return terminator instruction") + } + }; - function.dfg[predecessor_block].set_terminator(redirected_terminator_instruction); - cfg.recompute_block(function, predecessor_block); - } - cfg.recompute_block(function, block); + function.dfg[predecessor_block].set_terminator(redirected_terminator_instruction); + cfg.recompute_block(function, predecessor_block); } + cfg.recompute_block(function, block); } /// If the given block has block parameters, replace them with the jump arguments from the predecessor.