chore(ssa refactor): Fix loading from mutable parameters

crates/noirc_evaluator/src/ssa_refactor/ir/basic_block.rs

@@ -77,4 +77,13 @@ impl BasicBlock {
             None => vec![].into_iter(),
         }
     }
+
+    /// Removes the given instruction from this block if present or panics otherwise.
+    pub(crate) fn remove_instruction(&mut self, instruction: InstructionId) {
+        let index =
+            self.instructions.iter().position(|id| *id == instruction).unwrap_or_else(|| {
+                panic!("remove_instruction: No such instruction {instruction:?} in block")
+            });
+        self.instructions.remove(index);
+    }
 }

crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs

@@ -25,7 +25,7 @@ impl std::fmt::Display for Intrinsic {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             Intrinsic::Println => write!(f, "println"),
-            Intrinsic::Sort => write!(f, "sort"),
+            Intrinsic::Sort => write!(f, "arraysort"),
             Intrinsic::ToBits(Endian::Big) => write!(f, "to_be_bits"),
             Intrinsic::ToBits(Endian::Little) => write!(f, "to_le_bits"),
             Intrinsic::ToRadix(Endian::Big) => write!(f, "to_be_radix"),
@@ -39,7 +39,7 @@ impl Intrinsic {
     pub(crate) fn lookup(name: &str) -> Option<Intrinsic> {
         match name {
             "println" => Some(Intrinsic::Println),
-            "array_sort" => Some(Intrinsic::Sort),
+            "arraysort" => Some(Intrinsic::Sort),
             "to_le_radix" => Some(Intrinsic::ToRadix(Endian::Little)),
             "to_be_radix" => Some(Intrinsic::ToRadix(Endian::Big)),
             "to_le_bits" => Some(Intrinsic::ToBits(Endian::Little)),

crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs

@@ -143,7 +143,7 @@ pub(crate) fn display_instruction(
         Instruction::Allocate { size } => writeln!(f, "alloc {size} fields"),
         Instruction::Load { address } => writeln!(f, "load {}", show(*address)),
         Instruction::Store { address, value } => {
-            writeln!(f, "store {} at {}", show(*address), show(*value))
+            writeln!(f, "store {} at {}", show(*value), show(*address))
         }
     }
 }

crates/noirc_evaluator/src/ssa_refactor/ir/value.rs

@@ -12,7 +12,7 @@ pub(crate) type ValueId = Id<Value>;
 
 /// Value is the most basic type allowed in the IR.
 /// Transition Note: A Id<Value> is similar to `NodeId` in our previous IR.
-#[derive(Debug, PartialEq, Eq, Hash, Clone)]
+#[derive(Debug, PartialEq, Eq, Hash, Copy, Clone)]
 pub(crate) enum Value {
     /// This value was created due to an instruction
     ///

crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs

@@ -8,7 +8,10 @@ use crate::ssa_refactor::ir::{
     value::{Value, ValueId},
 };
 
-use super::{ir::instruction::Intrinsic, ssa_gen::Ssa};
+use super::{
+    ir::instruction::{InstructionId, Intrinsic},
+    ssa_gen::Ssa,
+};
 
 /// The per-function context for each ssa function being generated.
 ///
@@ -18,7 +21,7 @@ use super::{ir::instruction::Intrinsic, ssa_gen::Ssa};
 /// Contrary to the name, this struct has the capacity to build as many
 /// functions as needed, although it is limited to one function at a time.
 pub(crate) struct FunctionBuilder {
-    current_function: Function,
+    pub(super) current_function: Function,
     current_block: BasicBlockId,
     finished_functions: Vec<Function>,
 }
@@ -114,12 +117,7 @@ impl FunctionBuilder {
         offset: ValueId,
         type_to_load: Type,
     ) -> ValueId {
-        if let Some(offset) = self.current_function.dfg.get_numeric_constant(offset) {
-            if !offset.is_zero() {
-                let offset = self.field_constant(offset);
-                address = self.insert_binary(address, BinaryOp::Add, offset);
-            }
-        };
+        address = self.insert_binary(address, BinaryOp::Add, offset);
         self.insert_instruction(Instruction::Load { address }, Some(vec![type_to_load]))[0]
     }
 
@@ -205,32 +203,6 @@ impl FunctionBuilder {
         self.terminate_block_with(TerminatorInstruction::Return { return_values });
     }
 
-    /// Mutates a load instruction into a store instruction.
-    ///
-    /// This function is used while generating ssa-form for assignments currently.
-    /// To re-use most of the expression infrastructure, the lvalue of an assignment
-    /// is compiled as an expression and to assign to it we replace the final load
-    /// (which should always be present to load a mutable value) with a store of the
-    /// assigned value.
-    pub(crate) fn mutate_load_into_store(&mut self, load_result: ValueId, value_to_store: ValueId) {
-        let (instruction, address) = match &self.current_function.dfg[load_result] {
-            Value::Instruction { instruction, .. } => {
-                match &self.current_function.dfg[*instruction] {
-                    Instruction::Load { address } => (*instruction, *address),
-                    other => {
-                        panic!("mutate_load_into_store: Expected Load instruction, found {other:?}")
-                    }
-                }
-            }
-            other => panic!("mutate_load_into_store: Expected Load instruction, found {other:?}"),
-        };
-
-        let store = Instruction::Store { address, value: value_to_store };
-        self.current_function.dfg.replace_instruction(instruction, store);
-        // Clear the results of the previous load for safety
-        self.current_function.dfg.make_instruction_results(instruction, None);
-    }
-
     /// Returns a ValueId pointing to the given function or imports the function
     /// into the current function if it was not already, and returns that ID.
     pub(crate) fn import_function(&mut self, function: FunctionId) -> ValueId {
@@ -243,4 +215,25 @@ impl FunctionBuilder {
         Intrinsic::lookup(name)
             .map(|intrinsic| self.current_function.dfg.import_intrinsic(intrinsic))
     }
+
+    /// Removes the given instruction from the current block or panics otherwise.
+    pub(crate) fn remove_instruction_from_current_block(&mut self, instruction: InstructionId) {
+        self.current_function.dfg[self.current_block].remove_instruction(instruction);
+    }
+}
+
+impl std::ops::Index<ValueId> for FunctionBuilder {
+    type Output = Value;
+
+    fn index(&self, id: ValueId) -> &Self::Output {
+        &self.current_function.dfg[id]
+    }
+}
+
+impl std::ops::Index<InstructionId> for FunctionBuilder {
+    type Output = Instruction;
+
+    fn index(&self, id: InstructionId) -> &Self::Output {
+        &self.current_function.dfg[id]
+    }
 }

crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs

@@ -14,7 +14,7 @@ use crate::ssa_refactor::ir::types::Type;
 use crate::ssa_refactor::ir::value::ValueId;
 use crate::ssa_refactor::ssa_builder::FunctionBuilder;
 
-use super::value::{Tree, Values};
+use super::value::{Tree, Value, Values};
 
 // TODO: Make this a threadsafe queue so we can compile functions in parallel
 type FunctionQueue = Vec<(ast::FuncId, IrFunctionId)>;
@@ -63,23 +63,42 @@ impl<'a> FunctionContext<'a> {
     /// The returned parameter type list will be flattened, so any struct parameters will
     /// be returned as one entry for each field (recursively).
     fn add_parameters_to_scope(&mut self, parameters: &Parameters) {
-        for (id, _, _, typ) in parameters {
-            self.add_parameter_to_scope(*id, typ);
+        for (id, mutable, _, typ) in parameters {
+            self.add_parameter_to_scope(*id, typ, *mutable);
         }
     }
 
     /// Adds a "single" parameter to scope.
     ///
     /// Single is in quotes here because in the case of tuple parameters, the tuple is flattened
     /// into a new parameter for each field recursively.
-    fn add_parameter_to_scope(&mut self, parameter_id: LocalId, parameter_type: &ast::Type) {
+    fn add_parameter_to_scope(
+        &mut self,
+        parameter_id: LocalId,
+        parameter_type: &ast::Type,
+        mutable: bool,
+    ) {
         // Add a separate parameter for each field type in 'parameter_type'
-        let parameter_value =
-            Self::map_type(parameter_type, |typ| self.builder.add_parameter(typ).into());
+        let parameter_value = Self::map_type(parameter_type, |typ| {
+            let value = self.builder.add_parameter(typ);
+            if mutable {
+                self.new_mutable_variable(value)
+            } else {
+                value.into()
+            }
+        });
 
         self.definitions.insert(parameter_id, parameter_value);
     }
 
+    /// Create a new mutable storage slot and store into it the given initial value of the variable.
+    pub(super) fn new_mutable_variable(&mut self, value_to_store: ValueId) -> Value {
+        let alloc = self.builder.insert_allocate(1);
+        self.builder.insert_store(alloc, value_to_store);
+        let typ = self.builder.type_of_value(value_to_store);
+        Value::Mutable(alloc, typ)
+    }
+
     /// Maps the given type to a Tree of the result type.
     ///
     /// This can be used to (for example) flatten a tuple type, creating
@@ -224,12 +243,8 @@ impl<'a> FunctionContext<'a> {
                 }
             }
             (Tree::Leaf(lhs), Tree::Leaf(rhs)) => {
-                // Re-evaluating these should have no effect
-                let (lhs, rhs) = (lhs.eval(self), rhs.eval(self));
-
-                // Expect lhs to be previously evaluated. If it is a load we need to undo
-                // the load to get the address to store to.
-                self.builder.mutate_load_into_store(lhs, rhs);
+                let (lhs, rhs) = (lhs.eval_reference(), rhs.eval(self));
+                self.builder.insert_store(lhs, rhs);
             }
             (lhs, rhs) => {
                 unreachable!(

crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs

@@ -161,16 +161,21 @@ impl<'a> FunctionContext<'a> {
 
     fn codegen_index(&mut self, index: &ast::Index) -> Values {
         let array = self.codegen_non_tuple_expression(&index.collection);
-        self.codegen_array_index(array, &index.index, &index.element_type)
+        self.codegen_array_index(array, &index.index, &index.element_type, true)
     }
 
     /// This is broken off from codegen_index so that it can also be
-    /// used to codegen a LValue::Index
+    /// used to codegen a LValue::Index.
+    ///
+    /// Set load_result to true to load from each relevant index of the array
+    /// (it may be multiple in the case of tuples). Set it to false to instead
+    /// return a reference to each element, for use with the store instruction.
     fn codegen_array_index(
         &mut self,
         array: super::ir::value::ValueId,
         index: &ast::Expression,
         element_type: &ast::Type,
+        load_result: bool,
     ) -> Values {
         let base_offset = self.codegen_non_tuple_expression(index);
 
@@ -183,7 +188,12 @@ impl<'a> FunctionContext<'a> {
         Self::map_type(element_type, |typ| {
             let offset = self.make_offset(base_index, field_index);
             field_index += 1;
-            self.builder.insert_load(array, offset, typ).into()
+            if load_result {
+                self.builder.insert_load(array, offset, typ)
+            } else {
+                self.builder.insert_binary(array, BinaryOp::Add, offset)
+            }
+            .into()
         })
     }
 
@@ -292,10 +302,7 @@ impl<'a> FunctionContext<'a> {
         if let_expr.mutable {
             values.map_mut(|value| {
                 let value = value.eval(self);
-                // Size is always 1 here since we're recursively unpacking tuples
-                let alloc = self.builder.insert_allocate(1);
-                self.builder.insert_store(alloc, value);
-                alloc.into()
+                Tree::Leaf(self.new_mutable_variable(value))
             });
         }
 
@@ -312,16 +319,28 @@ impl<'a> FunctionContext<'a> {
     fn codegen_assign(&mut self, assign: &ast::Assign) -> Values {
         let lhs = self.codegen_lvalue(&assign.lvalue);
         let rhs = self.codegen_expression(&assign.expression);
+
         self.assign(lhs, rhs);
         self.unit_value()
     }
 
     fn codegen_lvalue(&mut self, lvalue: &ast::LValue) -> Values {
         match lvalue {
-            ast::LValue::Ident(ident) => self.codegen_ident(ident),
+            ast::LValue::Ident(ident) => {
+                // Do not .eval the Values here! We do not want to load from any references within
+                // since we want to return the references instead
+                match &ident.definition {
+                    ast::Definition::Local(id) => self.lookup(*id),
+                    other => panic!("Unexpected definition found for mutable value: {other}"),
+                }
+            }
             ast::LValue::Index { array, index, element_type, location: _ } => {
+                // Note that unlike the Ident case, we're .eval'ing the array here.
+                // This is because arrays are already references and thus a mutable reference
+                // to an array would be a Value::Mutable( Value::Mutable ( address ) ), and we
+                // only need the inner mutable value.
                 let array = self.codegen_lvalue(array).into_leaf().eval(self);
-                self.codegen_array_index(array, index, element_type)
+                self.codegen_array_index(array, index, element_type, false)
             }
             ast::LValue::MemberAccess { object, field_index } => {
                 let object = self.codegen_lvalue(object);

crates/noirc_evaluator/src/ssa_refactor/ssa_gen/value.rs

@@ -32,6 +32,15 @@ impl Value {
             }
         }
     }
+
+    /// Evaluates the value, returning a reference to the mutable variable found within
+    /// if possible. Compared to .eval, this method will not load from self if it is Value::Mutable.
+    pub(super) fn eval_reference(self) -> IrValueId {
+        match self {
+            Value::Normal(value) => value,
+            Value::Mutable(address, _) => address,
+        }
+    }
 }
 
 pub(super) type Values = Tree<Value>;