From 3aca9ed623d14e412f9520d33fafdf806dd3ba6a Mon Sep 17 00:00:00 2001 From: Tom French Date: Thu, 10 Aug 2023 22:40:44 +0100 Subject: [PATCH 1/7] feat: Perform dead instruction elimination on intrinsics --- .../noirc_evaluator/src/ssa/ir/instruction.rs | 21 +++++++++++++++ crates/noirc_evaluator/src/ssa/opt/die.rs | 26 ++++++++++++++----- 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction.rs b/crates/noirc_evaluator/src/ssa/ir/instruction.rs index 3b8ec03c125..ca1c64aa5e2 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction.rs @@ -68,6 +68,27 @@ impl std::fmt::Display for Intrinsic { } impl Intrinsic { + /// Returns whether the `Intrinsic` has side effects. + /// + /// If there are no side effects then the `Intrinsic` can be removed if the result is unused. + pub(crate) fn has_side_effects(&self) -> bool { + match self { + Intrinsic::Println => true, + + Intrinsic::Sort + | Intrinsic::ArrayLen + | Intrinsic::SlicePushBack + | Intrinsic::SlicePushFront + | Intrinsic::SlicePopBack + | Intrinsic::SlicePopFront + | Intrinsic::SliceInsert + | Intrinsic::SliceRemove + | Intrinsic::ToBits(_) + | Intrinsic::ToRadix(_) + | Intrinsic::BlackBox(_) => false, + } + } + /// Lookup an Intrinsic by name and return it if found. /// If there is no such intrinsic by that name, None is returned. pub(crate) fn lookup(name: &str) -> Option { diff --git a/crates/noirc_evaluator/src/ssa/opt/die.rs b/crates/noirc_evaluator/src/ssa/opt/die.rs index 935568af2db..0c0730aa293 100644 --- a/crates/noirc_evaluator/src/ssa/opt/die.rs +++ b/crates/noirc_evaluator/src/ssa/opt/die.rs @@ -92,12 +92,26 @@ impl Context { let instruction = &function.dfg[instruction_id]; - // These instruction types cannot be removed - if matches!( - instruction, - Constrain(_) | Call { .. } | Store { .. } | EnableSideEffects { .. } - ) { - return false; + match instruction { + // These instruction types can be safely removed + Binary(_) + | Cast(_, _) + | Not(_) + | Truncate { .. } + | Allocate + | Load { .. } + | ArrayGet { .. } + | ArraySet { .. } => (), + + // These instruction types cannot be removed + Constrain(_) | Store { .. } | EnableSideEffects { .. } => return false, + + Call { func, .. } => match function.dfg[*func] { + Value::Intrinsic(intrinsic) if !intrinsic.has_side_effects() => { + // Intrinsics without side-effects can be safely removed. + } + _ => return false, + }, } let results = function.dfg.instruction_results(instruction_id); From 14187275c95322e7bbfdaa2f52afd8430a97ad60 Mon Sep 17 00:00:00 2001 From: Tom French Date: Thu, 10 Aug 2023 22:48:23 +0100 Subject: [PATCH 2/7] chore: add test case for dead instruction elimination on intrinsics --- .../tests/compile_success/intrinsic_die/Nargo.toml | 7 +++++++ .../tests/compile_success/intrinsic_die/src/main.nr | 9 +++++++++ 2 files changed, 16 insertions(+) create mode 100644 crates/nargo_cli/tests/compile_success/intrinsic_die/Nargo.toml create mode 100644 crates/nargo_cli/tests/compile_success/intrinsic_die/src/main.nr diff --git a/crates/nargo_cli/tests/compile_success/intrinsic_die/Nargo.toml b/crates/nargo_cli/tests/compile_success/intrinsic_die/Nargo.toml new file mode 100644 index 00000000000..60e0310eab2 --- /dev/null +++ b/crates/nargo_cli/tests/compile_success/intrinsic_die/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "intrinsic_die" +type = "bin" +authors = [""] +compiler_version = "0.6.0" + +[dependencies] diff --git a/crates/nargo_cli/tests/compile_success/intrinsic_die/src/main.nr b/crates/nargo_cli/tests/compile_success/intrinsic_die/src/main.nr new file mode 100644 index 00000000000..5c59d2c8028 --- /dev/null +++ b/crates/nargo_cli/tests/compile_success/intrinsic_die/src/main.nr @@ -0,0 +1,9 @@ +use dep::std; + +// This test checks that we perform dead-instruction-elimination on intrinsic functions. + +fn main(x: Field) { + let bytes = x.to_be_bytes(32); + + let hash = std::hash::pedersen([x]); +} From e9e61f175a7411c2e67c22642bd5abb29cf8e618 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 15 Aug 2023 19:42:15 +0100 Subject: [PATCH 3/7] fix: update to address `BlackBoxFunc::RecursiveAggregation` having side effects --- crates/noirc_evaluator/src/ssa/ir/instruction.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction.rs b/crates/noirc_evaluator/src/ssa/ir/instruction.rs index ca1c64aa5e2..11fe5e0bf5e 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction.rs @@ -84,8 +84,10 @@ impl Intrinsic { | Intrinsic::SliceInsert | Intrinsic::SliceRemove | Intrinsic::ToBits(_) - | Intrinsic::ToRadix(_) - | Intrinsic::BlackBox(_) => false, + | Intrinsic::ToRadix(_) => false, + + // Some black box functions have side-effects + Intrinsic::BlackBox(func) => matches!(func, BlackBoxFunc::RecursiveAggregation), } } From a66e0e3f5617fa8ee6d4c30079526c8e54e8e7e7 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 15 Aug 2023 19:48:39 +0100 Subject: [PATCH 4/7] chore: move `match` statement inside `Instruction` --- .../noirc_evaluator/src/ssa/ir/instruction.rs | 23 +++++++++++++ crates/noirc_evaluator/src/ssa/opt/die.rs | 33 ++++--------------- 2 files changed, 30 insertions(+), 26 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction.rs b/crates/noirc_evaluator/src/ssa/ir/instruction.rs index 11fe5e0bf5e..cf202f1463f 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction.rs @@ -201,6 +201,29 @@ impl Instruction { matches!(self.result_type(), InstructionResultType::Unknown) } + pub(crate) fn has_side_effects(&self, dfg: &DataFlowGraph) -> bool { + use Instruction::*; + + match self { + Binary(_) + | Cast(_, _) + | Not(_) + | Truncate { .. } + | Allocate + | Load { .. } + | ArrayGet { .. } + | ArraySet { .. } => false, + + Constrain(_) | Store { .. } | EnableSideEffects { .. } => true, + + // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is. + Call { func, .. } => match dfg[*func] { + Value::Intrinsic(intrinsic) => intrinsic.has_side_effects(), + _ => false, + }, + } + } + /// Maps each ValueId inside this instruction to a new ValueId, returning the new instruction. /// Note that the returned instruction is fresh and will not have an assigned InstructionId /// until it is manually inserted in a DataFlowGraph later. diff --git a/crates/noirc_evaluator/src/ssa/opt/die.rs b/crates/noirc_evaluator/src/ssa/opt/die.rs index 0c0730aa293..d38d3dde0a5 100644 --- a/crates/noirc_evaluator/src/ssa/opt/die.rs +++ b/crates/noirc_evaluator/src/ssa/opt/die.rs @@ -7,7 +7,7 @@ use crate::ssa::{ basic_block::{BasicBlock, BasicBlockId}, dfg::DataFlowGraph, function::Function, - instruction::{Instruction, InstructionId}, + instruction::InstructionId, post_order::PostOrder, value::{Value, ValueId}, }, @@ -88,34 +88,15 @@ impl Context { /// An instruction can be removed as long as it has no side-effects, and none of its result /// values have been referenced. fn is_unused(&self, instruction_id: InstructionId, function: &Function) -> bool { - use Instruction::*; - let instruction = &function.dfg[instruction_id]; - match instruction { - // These instruction types can be safely removed - Binary(_) - | Cast(_, _) - | Not(_) - | Truncate { .. } - | Allocate - | Load { .. } - | ArrayGet { .. } - | ArraySet { .. } => (), - - // These instruction types cannot be removed - Constrain(_) | Store { .. } | EnableSideEffects { .. } => return false, - - Call { func, .. } => match function.dfg[*func] { - Value::Intrinsic(intrinsic) if !intrinsic.has_side_effects() => { - // Intrinsics without side-effects can be safely removed. - } - _ => return false, - }, + if instruction.has_side_effects(&function.dfg) { + // If the instruction has side effects we should never remove it. + false + } else { + let results = function.dfg.instruction_results(instruction_id); + results.iter().all(|result| !self.used_values.contains(result)) } - - let results = function.dfg.instruction_results(instruction_id); - results.iter().all(|result| !self.used_values.contains(result)) } /// Adds values referenced by the terminator to the set of used values. From e9fe52d7ef03841985737fc693b12b142fd6f503 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 15 Aug 2023 20:24:09 +0100 Subject: [PATCH 5/7] chore: handle `Intrinsic::AssertConstant` --- crates/noirc_evaluator/src/ssa/ir/instruction.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction.rs b/crates/noirc_evaluator/src/ssa/ir/instruction.rs index 824c4343075..e49bea0a924 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction.rs @@ -75,7 +75,7 @@ impl Intrinsic { /// If there are no side effects then the `Intrinsic` can be removed if the result is unused. pub(crate) fn has_side_effects(&self) -> bool { match self { - Intrinsic::Println => true, + Intrinsic::Println | Intrinsic::AssertConstant => true, Intrinsic::Sort | Intrinsic::ArrayLen From 6f24de722f6b3be5c4708d9a98b309b08001c930 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 15 Aug 2023 21:17:57 +0100 Subject: [PATCH 6/7] chore: use proper `Intrinsic` for println in DIE test --- crates/noirc_evaluator/src/ssa/opt/die.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa/opt/die.rs b/crates/noirc_evaluator/src/ssa/opt/die.rs index d38d3dde0a5..ee78fa82256 100644 --- a/crates/noirc_evaluator/src/ssa/opt/die.rs +++ b/crates/noirc_evaluator/src/ssa/opt/die.rs @@ -129,7 +129,12 @@ impl Context { #[cfg(test)] mod test { use crate::ssa::{ - ir::{function::RuntimeType, instruction::BinaryOp, map::Id, types::Type}, + ir::{ + function::RuntimeType, + instruction::{BinaryOp, Intrinsic}, + map::Id, + types::Type, + }, ssa_builder::FunctionBuilder, }; @@ -154,7 +159,6 @@ mod test { // return v9 // } let main_id = Id::test_new(0); - let println_id = Id::test_new(1); // Compiling main let mut builder = FunctionBuilder::new("main".into(), main_id, RuntimeType::Acir); @@ -182,6 +186,8 @@ mod test { let v9 = builder.insert_binary(v7, BinaryOp::Add, two); let v10 = builder.insert_binary(v7, BinaryOp::Add, three); let _v11 = builder.insert_binary(v10, BinaryOp::Add, v10); + + let println_id = builder.import_intrinsic_id(Intrinsic::Println); builder.insert_call(println_id, vec![v8], vec![]); builder.terminate_with_return(vec![v9]); From 4f1ecea74e0e5fe782a3a1f1d22b3f18a123f143 Mon Sep 17 00:00:00 2001 From: Tom French Date: Tue, 15 Aug 2023 21:22:43 +0100 Subject: [PATCH 7/7] chore: add `Intrinsic::StrAsBytes` to match --- crates/noirc_evaluator/src/ssa/ir/instruction.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction.rs b/crates/noirc_evaluator/src/ssa/ir/instruction.rs index a6b5a037805..62c273776ee 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction.rs @@ -87,6 +87,7 @@ impl Intrinsic { | Intrinsic::SlicePopFront | Intrinsic::SliceInsert | Intrinsic::SliceRemove + | Intrinsic::StrAsBytes | Intrinsic::ToBits(_) | Intrinsic::ToRadix(_) => false,