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]); +} diff --git a/crates/noirc_evaluator/src/ssa/ir/instruction.rs b/crates/noirc_evaluator/src/ssa/ir/instruction.rs index e09e21f158f..62c273776ee 100644 --- a/crates/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa/ir/instruction.rs @@ -72,6 +72,30 @@ 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 | Intrinsic::AssertConstant => true, + + Intrinsic::Sort + | Intrinsic::ArrayLen + | Intrinsic::SlicePushBack + | Intrinsic::SlicePushFront + | Intrinsic::SlicePopBack + | Intrinsic::SlicePopFront + | Intrinsic::SliceInsert + | Intrinsic::SliceRemove + | Intrinsic::StrAsBytes + | Intrinsic::ToBits(_) + | Intrinsic::ToRadix(_) => false, + + // Some black box functions have side-effects + Intrinsic::BlackBox(func) => matches!(func, BlackBoxFunc::RecursiveAggregation), + } + } + /// 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 { @@ -184,6 +208,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 935568af2db..ee78fa82256 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,20 +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]; - // These instruction types cannot be removed - if matches!( - instruction, - Constrain(_) | Call { .. } | Store { .. } | EnableSideEffects { .. } - ) { - 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. @@ -134,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, }; @@ -159,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); @@ -187,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]);