Merge a03b7da into ade69a9

noir-lang · Aug 5, 2024 · 28985b0 · 28985b0
2 parents ade69a9 + a03b7da
commit 28985b0
Show file tree

Hide file tree

Showing 2 changed files with 192 additions and 16 deletions.
diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs
@@ -334,6 +334,38 @@ impl Instruction {
     pub(crate) fn can_eliminate_if_unused(&self, dfg: &DataFlowGraph) -> bool {
         use Instruction::*;
         match self {
+            Cast(_, _) | Not(_) | Truncate { .. } | Allocate | Load { .. } | IfElse { .. } => true,
+
+            Constrain(..)
+            | Store { .. }
+            | EnableSideEffects { .. }
+            | IncrementRc { .. }
+            | DecrementRc { .. }
+            | RangeCheck { .. } => false,
+
+            ArrayGet { array, index } | ArraySet { array, index, .. } => {
+                if let Some(array_length) = dfg.try_get_array_length(*array) {
+                    if let Some(known_index) = dfg.get_numeric_constant(*index) {
+                        // If the index is known at compile-time, we can only remove it if it's not out of bounds.
+                        // If it's out of bounds we'd like that to keep failing at runtime.
+                        known_index < array_length.into()
+                    } else {
+                        // If the index is not known at compile-time we can't remove this instruction as this
+                        // might be an index out of bounds.
+                        false
+                    }
+                } else {
+                    if let ArrayGet { .. } = self {
+                        // array_get on a slice always does an index in bounds check,
+                        // so we can remove this instruction if it's unused
+                        true
+                    } else {
+                        // The same check isn't done on array_set, though
+                        false
+                    }
+                }
+            }
+
             Binary(binary) => {
                 if matches!(binary.operator, BinaryOp::Div | BinaryOp::Mod) {
                     if let Some(rhs) = dfg.get_numeric_constant(binary.rhs) {
@@ -345,21 +377,6 @@ impl Instruction {
                     true
                 }
             }
-            Cast(_, _)
-            | Not(_)
-            | Truncate { .. }
-            | Allocate
-            | Load { .. }
-            | ArrayGet { .. }
-            | IfElse { .. }
-            | ArraySet { .. } => true,
-
-            Constrain(..)
-            | Store { .. }
-            | EnableSideEffects { .. }
-            | IncrementRc { .. }
-            | DecrementRc { .. }
-            | RangeCheck { .. } => false,
 
             // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is.
             Call { func, .. } => match dfg[*func] {

diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs
@@ -170,12 +170,14 @@ impl Context {
 
 #[cfg(test)]
 mod test {
+    use std::rc::Rc;
+
     use crate::ssa::{
         function_builder::FunctionBuilder,
         ir::{
             instruction::{BinaryOp, Intrinsic},
             map::Id,
-            types::Type,
+            types::{NumericType, Type},
         },
     };
 
@@ -303,4 +305,161 @@ mod test {
 
         assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1);
     }
+
+    #[test]
+    fn does_not_remove_array_get_if_index_known_at_compile_time_is_out_of_bounds() {
+        // fn main f0 {
+        //   b0(v0: [Field; 1]):
+        //     v1 = array_get v0, index u32 10
+        //     return
+        // }
+        let main_id = Id::test_new(0);
+
+        // Compiling main
+        let mut builder = FunctionBuilder::new("main".into(), main_id);
+
+        let v0 = builder.add_parameter(Type::Array(Rc::new(vec![Type::field()]), 1));
+
+        let const_ten = builder.numeric_constant(10_u128, Type::Numeric(NumericType::NativeField));
+        builder.insert_array_get(v0, const_ten, Type::field());
+        builder.terminate_with_return(Vec::new());
+
+        let ssa = builder.finish();
+        println!("{}", ssa);
+
+        let main = ssa.main();
+        let instructions_before = main.dfg[main.entry_block()].instructions().len();
+
+        let ssa = ssa.dead_instruction_elimination();
+        let main = ssa.main();
+        let instructions_after = main.dfg[main.entry_block()].instructions().len();
+
+        assert_eq!(instructions_after, instructions_before);
+    }
+
+    #[test]
+    fn remove_array_get_if_index_known_at_compile_time_is_not_out_of_bounds() {
+        // fn main f0 {
+        //   b0(v0: [Field; 1]):
+        //     v1 = array_get v0, index u32 0
+        //     return
+        // }
+        let main_id = Id::test_new(0);
+
+        // Compiling main
+        let mut builder = FunctionBuilder::new("main".into(), main_id);
+
+        let v0 = builder.add_parameter(Type::Array(Rc::new(vec![Type::field()]), 1));
+
+        let const_zero = builder.numeric_constant(0_u128, Type::Numeric(NumericType::NativeField));
+        builder.insert_array_get(v0, const_zero, Type::field());
+        builder.terminate_with_return(Vec::new());
+
+        let ssa = builder.finish();
+        println!("{}", ssa);
+
+        let main = ssa.main();
+        let instructions_before = main.dfg[main.entry_block()].instructions().len();
+
+        let ssa = ssa.dead_instruction_elimination();
+        let main = ssa.main();
+        let instructions_after = main.dfg[main.entry_block()].instructions().len();
+
+        assert_eq!(instructions_after, instructions_before - 1);
+    }
+
+    #[test]
+    fn does_not_remove_array_get_if_index_is_not_known_at_compile_time() {
+        // fn main f0 {
+        //   b0(v0: [Field; 1], v1: Field):
+        //     v2 = array_get v0, index v1
+        //     return
+        // }
+        let main_id = Id::test_new(0);
+
+        // Compiling main
+        let mut builder = FunctionBuilder::new("main".into(), main_id);
+
+        let v0 = builder.add_parameter(Type::Array(Rc::new(vec![Type::field()]), 1));
+        let v1 = builder.add_parameter(Type::field());
+
+        builder.insert_array_get(v0, v1, Type::field());
+        builder.terminate_with_return(Vec::new());
+
+        let ssa = builder.finish();
+        println!("{}", ssa);
+
+        let main = ssa.main();
+        let instructions_before = main.dfg[main.entry_block()].instructions().len();
+
+        let ssa = ssa.dead_instruction_elimination();
+        let main = ssa.main();
+        let instructions_after = main.dfg[main.entry_block()].instructions().len();
+
+        assert_eq!(instructions_after, instructions_before);
+    }
+
+    #[test]
+    fn does_not_remove_array_set_if_index_known_at_compile_time_is_out_of_bounds() {
+        // fn main f0 {
+        //   b0(v0: [Field; 1]):
+        //     v1 = array_set v0, index u32 10, value Field 1
+        //     return
+        // }
+        let main_id = Id::test_new(0);
+
+        // Compiling main
+        let mut builder = FunctionBuilder::new("main".into(), main_id);
+
+        let v0 = builder.add_parameter(Type::Array(Rc::new(vec![Type::field()]), 1));
+
+        let const_one = builder.numeric_constant(1_u128, Type::Numeric(NumericType::NativeField));
+        let const_ten = builder.numeric_constant(10_u128, Type::Numeric(NumericType::NativeField));
+        builder.insert_array_set(v0, const_ten, const_one);
+        builder.terminate_with_return(Vec::new());
+
+        let ssa = builder.finish();
+        println!("{}", ssa);
+
+        let main = ssa.main();
+        let instructions_before = main.dfg[main.entry_block()].instructions().len();
+
+        let ssa = ssa.dead_instruction_elimination();
+        let main = ssa.main();
+        let instructions_after = main.dfg[main.entry_block()].instructions().len();
+
+        assert_eq!(instructions_after, instructions_before);
+    }
+
+    #[test]
+    fn does_not_remove_array_set_for_slices() {
+        // fn main f0 {
+        //   b0(v0: [Field]):
+        //     v1 = array_set v0, index u32 10, value Field 1
+        //     return
+        // }
+        let main_id = Id::test_new(0);
+
+        // Compiling main
+        let mut builder = FunctionBuilder::new("main".into(), main_id);
+
+        let v0 = builder.add_parameter(Type::Slice(Rc::new(vec![Type::field()])));
+
+        let const_one = builder.numeric_constant(1_u128, Type::Numeric(NumericType::NativeField));
+        let const_ten = builder.numeric_constant(10_u128, Type::Numeric(NumericType::NativeField));
+        builder.insert_array_set(v0, const_ten, const_one);
+        builder.terminate_with_return(Vec::new());
+
+        let ssa = builder.finish();
+        println!("{}", ssa);
+
+        let main = ssa.main();
+        let instructions_before = main.dfg[main.entry_block()].instructions().len();
+
+        let ssa = ssa.dead_instruction_elimination();
+        let main = ssa.main();
+        let instructions_after = main.dfg[main.entry_block()].instructions().len();
+
+        assert_eq!(instructions_after, instructions_before);
+    }
 }