chore(ssa refactor): Update how instruction result types are retrieved (

#1222)

* Implement binary instructions

* Cleanup PR

* Change how instruction result types are handled

* Reorganize make_instruction flow a bit

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

@@ -2,7 +2,7 @@ use super::{
     basic_block::{BasicBlock, BasicBlockId},
     constant::{NumericConstant, NumericConstantId},
     function::Signature,
-    instruction::{Instruction, InstructionId},
+    instruction::{Instruction, InstructionId, InstructionResultType},
     map::{DenseMap, Id, SecondaryMap, TwoWayMap},
     types::Type,
     value::{Value, ValueId},
@@ -110,11 +110,19 @@ impl DataFlowGraph {
     }
 
     /// Inserts a new instruction into the DFG.
-    /// This does not add the instruction to the block or populate the instruction's result list
-    pub(crate) fn make_instruction(&mut self, instruction_data: Instruction) -> InstructionId {
+    /// This does not add the instruction to the block.
+    /// Returns the id of the new instruction and its results.
+    ///
+    /// Populates the instruction's results with the given ctrl_typevars if the instruction
+    /// is a Load, Call, or Intrinsic. Otherwise the instruction's results will be known
+    /// by the instruction itself and None can safely be passed for this parameter.
+    pub(crate) fn make_instruction(
+        &mut self,
+        instruction_data: Instruction,
+        ctrl_typevars: Option<Vec<Type>>,
+    ) -> InstructionId {
         let id = self.instructions.insert(instruction_data);
-        // Create a new vector to store the potential results for the instruction.
-        self.results.insert(id, Default::default());
+        self.make_instruction_results(id, ctrl_typevars);
         id
     }
 
@@ -134,46 +142,51 @@ impl DataFlowGraph {
     /// Attaches results to the instruction, clearing any previous results.
     ///
     /// Returns the results of the instruction
-    pub(crate) fn make_instruction_results(
+    fn make_instruction_results(
         &mut self,
         instruction_id: InstructionId,
-        ctrl_typevar: Type,
-    ) -> &[ValueId] {
-        // Clear all of the results instructions associated with this
-        // instruction.
-        self.results.get_mut(&instruction_id).expect("all instructions should have a `result` allocation when instruction was added to the DFG").clear();
+        ctrl_typevars: Option<Vec<Type>>,
+    ) {
+        self.results.insert(instruction_id, Default::default());
 
         // Get all of the types that this instruction produces
         // and append them as results.
-        let typs = self.instruction_result_types(instruction_id, ctrl_typevar);
+        let typs = self.instruction_result_types(instruction_id, ctrl_typevars);
 
         for typ in typs {
             self.append_result(instruction_id, typ);
         }
-
-        self.results.get_mut(&instruction_id)
-            .expect("all instructions should have a `result` allocation when instruction was added to the DFG")
-            .as_slice()
     }
 
     /// Return the result types of this instruction.
     ///
-    /// For example, an addition instruction will return
-    /// one type which is the type of the operands involved.
-    /// This is the `ctrl_typevar` in this case.
+    /// In the case of Load, Call, and Intrinsic, the function's result
+    /// type may be unknown. In this case, the given ctrl_typevars are returned instead.
+    /// ctrl_typevars is taken in as an Option since it is common to omit them when getting
+    /// the type of an instruction that does not require them. Compared to passing an empty Vec,
+    /// Option has the benefit of panicking if it is accidentally used for a Call instruction,
+    /// rather than silently returning the empty Vec and continuing.
     fn instruction_result_types(
         &self,
         instruction_id: InstructionId,
-        ctrl_typevar: Type,
+        ctrl_typevars: Option<Vec<Type>>,
     ) -> Vec<Type> {
-        // Check if it is a call instruction. If so, we don't support that yet
-        let ins_data = &self.instructions[instruction_id];
-        match ins_data {
-            Instruction::Call { .. } => todo!("function calls are not supported yet"),
-            ins => ins.return_types(ctrl_typevar),
+        let instruction = &self.instructions[instruction_id];
+        match instruction.result_type() {
+            InstructionResultType::Known(typ) => vec![typ],
+            InstructionResultType::Operand(value) => vec![self.type_of_value(value)],
+            InstructionResultType::None => vec![],
+            InstructionResultType::Unknown => {
+                ctrl_typevars.expect("Control typevars required but not given")
+            }
         }
     }
 
+    /// Returns the type of a given value
+    pub(crate) fn type_of_value(&self, value: ValueId) -> Type {
+        self.values[value].get_type()
+    }
+
     /// Appends a result type to the instruction.
     pub(crate) fn append_result(&mut self, instruction_id: InstructionId, typ: Type) -> ValueId {
         let results = self.results.get_mut(&instruction_id).unwrap();
@@ -257,21 +270,15 @@ impl std::ops::IndexMut<BasicBlockId> for DataFlowGraph {
 #[cfg(test)]
 mod tests {
     use super::DataFlowGraph;
-    use crate::ssa_refactor::ir::{
-        instruction::Instruction,
-        types::{NumericType, Type},
-    };
+    use crate::ssa_refactor::ir::instruction::Instruction;
 
     #[test]
     fn make_instruction() {
         let mut dfg = DataFlowGraph::default();
         let ins = Instruction::Allocate { size: 20 };
-        let ins_id = dfg.make_instruction(ins);
-
-        let num_results =
-            dfg.make_instruction_results(ins_id, Type::Numeric(NumericType::NativeField)).len();
+        let ins_id = dfg.make_instruction(ins, None);
 
         let results = dfg.instruction_results(ins_id);
-        assert_eq!(results.len(), num_results);
+        assert_eq!(results.len(), 1);
     }
 }

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

@@ -105,23 +105,39 @@ impl Instruction {
         }
     }
 
-    /// Returns the types that this instruction will return.
-    pub(crate) fn return_types(&self, ctrl_typevar: Type) -> Vec<Type> {
+    /// Returns the type that this instruction will return.
+    pub(crate) fn result_type(&self) -> InstructionResultType {
         match self {
-            Instruction::Binary(_) => vec![ctrl_typevar],
-            Instruction::Cast(_, typ) => vec![*typ],
-            Instruction::Not(_) => vec![ctrl_typevar],
-            Instruction::Truncate { .. } => vec![ctrl_typevar],
-            Instruction::Constrain(_) => vec![],
-            Instruction::Call { .. } => vec![],
-            Instruction::Intrinsic { .. } => vec![],
-            Instruction::Allocate { .. } => vec![Type::Reference],
-            Instruction::Load { .. } => vec![ctrl_typevar],
-            Instruction::Store { .. } => vec![],
+            Instruction::Binary(binary) => binary.result_type(),
+            Instruction::Cast(_, typ) => InstructionResultType::Known(*typ),
+            Instruction::Allocate { .. } => InstructionResultType::Known(Type::Reference),
+            Instruction::Not(value) | Instruction::Truncate { value, .. } => {
+                InstructionResultType::Operand(*value)
+            }
+            Instruction::Constrain(_) | Instruction::Store { .. } => InstructionResultType::None,
+            Instruction::Load { .. } | Instruction::Call { .. } | Instruction::Intrinsic { .. } => {
+                InstructionResultType::Unknown
+            }
         }
     }
 }
 
+/// The possible return values for Instruction::return_types
+pub(crate) enum InstructionResultType {
+    /// The result type of this instruction matches that of this operand
+    Operand(ValueId),
+
+    /// The result type of this instruction is known to be this type - independent of its operands.
+    Known(Type),
+
+    /// The result type of this function is unknown and separate from its operand types.
+    /// This occurs for function and intrinsic calls.
+    Unknown,
+
+    /// This instruction does not return any results.
+    None,
+}
+
 /// These are operations which can exit a basic block
 /// ie control flow type operations
 ///
@@ -169,6 +185,15 @@ pub(crate) struct Binary {
     pub(crate) operator: BinaryOp,
 }
 
+impl Binary {
+    pub(crate) fn result_type(&self) -> InstructionResultType {
+        match self.operator {
+            BinaryOp::Eq | BinaryOp::Lt => InstructionResultType::Known(Type::bool()),
+            _ => InstructionResultType::Operand(self.lhs),
+        }
+    }
+}
+
 /// Binary Operations allowed in the IR.
 /// Aside from the comparison operators (Eq and Lt), all operators
 /// will return the same type as their operands.

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

@@ -38,6 +38,10 @@ impl Type {
         Type::Numeric(NumericType::Unsigned { bit_size })
     }
 
+    pub(crate) fn bool() -> Type {
+        Type::unsigned(1)
+    }
+
     pub(crate) fn field() -> Type {
         Type::Numeric(NumericType::NativeField)
     }

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

@@ -28,3 +28,13 @@ pub(crate) enum Value {
     /// This Value originates from a numeric constant
     NumericConstant { constant: NumericConstantId, typ: Type },
 }
+
+impl Value {
+    pub(crate) fn get_type(&self) -> Type {
+        match self {
+            Value::Instruction { typ, .. } => *typ,
+            Value::Param { typ, .. } => *typ,
+            Value::NumericConstant { typ, .. } => *typ,
+        }
+    }
+}

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

@@ -3,7 +3,7 @@ use acvm::FieldElement;
 use crate::ssa_refactor::ir::{
     basic_block::BasicBlockId,
     function::{Function, FunctionId},
-    instruction::{Binary, BinaryOp, Instruction, InstructionId},
+    instruction::{Binary, BinaryOp, Instruction},
     types::Type,
     value::ValueId,
 };
@@ -71,33 +71,35 @@ impl<'ssa> FunctionBuilder<'ssa> {
         self.numeric_constant(value.into(), Type::field())
     }
 
-    fn insert_instruction(&mut self, instruction: Instruction) -> InstructionId {
-        let id = self.current_function.dfg.make_instruction(instruction);
+    fn insert_instruction(
+        &mut self,
+        instruction: Instruction,
+        ctrl_typevars: Option<Vec<Type>>,
+    ) -> &[ValueId] {
+        let id = self.current_function.dfg.make_instruction(instruction, ctrl_typevars);
         self.current_function.dfg.insert_instruction_in_block(self.current_block, id);
-        id
+        self.current_function.dfg.instruction_results(id)
     }
 
     /// Insert an allocate instruction at the end of the current block, allocating the
     /// given amount of field elements. Returns the result of the allocate instruction,
     /// which is always a Reference to the allocated data.
     pub(crate) fn insert_allocate(&mut self, size_to_allocate: u32) -> ValueId {
-        let id = self.insert_instruction(Instruction::Allocate { size: size_to_allocate });
-        self.current_function.dfg.make_instruction_results(id, Type::Reference)[0]
+        self.insert_instruction(Instruction::Allocate { size: size_to_allocate }, None)[0]
     }
 
     /// Insert a Load instruction at the end of the current block, loading from the given address
     /// which should point to a previous Allocate instruction. Note that this is limited to loading
     /// a single value. Loading multiple values (such as a tuple) will require multiple loads.
     /// Returns the element that was loaded.
     pub(crate) fn insert_load(&mut self, address: ValueId, type_to_load: Type) -> ValueId {
-        let id = self.insert_instruction(Instruction::Load { address });
-        self.current_function.dfg.make_instruction_results(id, type_to_load)[0]
+        self.insert_instruction(Instruction::Load { address }, Some(vec![type_to_load]))[0]
     }
 
     /// Insert a Store instruction at the end of the current block, storing the given element
     /// at the given address. Expects that the address points to a previous Allocate instruction.
     pub(crate) fn insert_store(&mut self, address: ValueId, value: ValueId) {
-        self.insert_instruction(Instruction::Store { address, value });
+        self.insert_instruction(Instruction::Store { address, value }, None);
     }
 
     /// Insert a binary instruction at the end of the current block.
@@ -107,16 +109,14 @@ impl<'ssa> FunctionBuilder<'ssa> {
         lhs: ValueId,
         operator: BinaryOp,
         rhs: ValueId,
-        typ: Type,
     ) -> ValueId {
-        let id = self.insert_instruction(Instruction::Binary(Binary { lhs, rhs, operator }));
-        self.current_function.dfg.make_instruction_results(id, typ)[0]
+        let instruction = Instruction::Binary(Binary { lhs, rhs, operator });
+        self.insert_instruction(instruction, None)[0]
     }
 
     /// Insert a not instruction at the end of the current block.
     /// Returns the result of the instruction.
-    pub(crate) fn insert_not(&mut self, rhs: ValueId, typ: Type) -> ValueId {
-        let id = self.insert_instruction(Instruction::Not(rhs));
-        self.current_function.dfg.make_instruction_results(id, typ)[0]
+    pub(crate) fn insert_not(&mut self, rhs: ValueId) -> ValueId {
+        self.insert_instruction(Instruction::Not(rhs), None)[0]
     }
 }

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

@@ -148,13 +148,10 @@ impl<'a> FunctionContext<'a> {
             std::mem::swap(&mut lhs, &mut rhs);
         }
 
-        // TODO: Rework how types are stored.
-        // They should be on values rather than on instruction results
-        let typ = Type::field();
-        let mut result = self.builder.insert_binary(lhs, op, rhs, typ);
+        let mut result = self.builder.insert_binary(lhs, op, rhs);
 
         if operator_requires_not(operator) {
-            result = self.builder.insert_not(result, typ);
+            result = self.builder.insert_not(result);
         }
         result.into()
     }

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

@@ -116,8 +116,8 @@ impl<'a> FunctionContext<'a> {
                 let address = if i == 0 {
                     array
                 } else {
-                    let offset = self.builder.numeric_constant((i as u128).into(), Type::field());
-                    self.builder.insert_binary(array, BinaryOp::Add, offset, Type::field())
+                    let offset = self.builder.field_constant(i as u128);
+                    self.builder.insert_binary(array, BinaryOp::Add, offset)
                 };
                 self.builder.insert_store(address, value.eval());
                 i += 1;