chore(ssa_refactor): Improve foreign call compilation (#1644)

* remove ForeignCall instruction from SSA * remove unnecessary Oracle RuntimeType * remove old comment
noir-lang · Jun 13, 2023 · e66fb0c · e66fb0c
1 parent c5da92c
commit e66fb0c
Show file tree

Hide file tree

Showing 10 changed files with 34 additions and 80 deletions.
diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen.rs b/crates/noirc_evaluator/src/brillig/brillig_gen.rs
@@ -158,20 +158,25 @@ impl BrilligGen {
 
                 self.context.not_instruction(condition, result_register);
             }
-            Instruction::ForeignCall { func, arguments } => {
-                let result_ids = dfg.instruction_results(instruction_id);
-
-                let input_registers =
-                    vecmap(arguments, |value_id| self.convert_ssa_value(*value_id, dfg));
-                let output_registers =
-                    vecmap(result_ids, |value_id| self.convert_ssa_value(*value_id, dfg));
-
-                self.context.foreign_call_instruction(
-                    func.to_owned(),
-                    &input_registers,
-                    &output_registers,
-                );
-            }
+            Instruction::Call { func, arguments } => match &dfg[*func] {
+                Value::ForeignFunction(func_name) => {
+                    let result_ids = dfg.instruction_results(instruction_id);
+
+                    let input_registers =
+                        vecmap(arguments, |value_id| self.convert_ssa_value(*value_id, dfg));
+                    let output_registers =
+                        vecmap(result_ids, |value_id| self.convert_ssa_value(*value_id, dfg));
+
+                    self.context.foreign_call_instruction(
+                        func_name.to_owned(),
+                        &input_registers,
+                        &output_registers,
+                    );
+                }
+                _ => {
+                    unreachable!("only foreign function calls supported in unconstrained functions")
+                }
+            },
             Instruction::Truncate { value, .. } => {
                 let result_ids = dfg.instruction_results(instruction_id);
                 let destination = self.get_or_create_register(result_ids[0]);
@@ -231,7 +236,6 @@ impl BrilligGen {
 
         brillig.convert_ssa_function(func);
 
-
         brillig.context.artifact()
     }
 

diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs
@@ -2,7 +2,7 @@
 
 use std::collections::HashMap;
 
-use crate::brillig::{Brillig, brillig_ir::artifact::BrilligArtifact};
+use crate::brillig::{brillig_ir::artifact::BrilligArtifact, Brillig};
 
 use self::acir_ir::{
     acir_variable::{AcirContext, AcirType, AcirVar},
@@ -190,9 +190,6 @@ impl Context {
                                     self.ssa_values.insert(*result, AcirValue::Var(output, result_acir_type));
                                 }
                             }
-                            RuntimeType::Oracle(_) => unimplemented!(
-                                "expected an intrinsic/brillig call, but found {func:?}. All Oracle methods should be wrapped in an unconstrained fn"
-                            ),
                         }
                     }
                     Value::Intrinsic(intrinsic) => {
@@ -212,6 +209,9 @@ impl Context {
                             self.ssa_values.insert(*result, output);
                         }
                     }
+                    Value::ForeignFunction(_) => unreachable!(
+                        "All `oracle` methods should be wrapped in an unconstrained fn"
+                    ),
                     _ => unreachable!("expected calling a function"),
                 }
             }
@@ -241,7 +241,6 @@ impl Context {
             Instruction::Load { .. } => {
                 unreachable!("Expected all load instructions to be removed before acir_gen")
             }
-            _ => unreachable!("instruction cannot be converted to ACIR"),
         }
     }
 

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs
@@ -196,6 +196,9 @@ impl DataFlowGraph {
 
     /// Gets or creates a ValueId for the given FunctionId.
     pub(crate) fn import_foreign_function(&mut self, function: &str) -> ValueId {
+        if let Some(existing) = self.foreign_functions.get(function) {
+            return *existing;
+        }
         self.values.insert(Value::ForeignFunction(function.to_owned()))
     }
 

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/function.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/function.rs
@@ -12,8 +12,6 @@ pub(crate) enum RuntimeType {
     Acir,
     // Unconstrained function, to be compiled to brillig and executed by the Brillig VM
     Brillig,
-    // Oracle function, to be compiled to a Brillig external/foreign call
-    Oracle(String),
 }
 /// A function holds a list of instructions.
 /// These instructions are further grouped into Basic blocks

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs
@@ -92,10 +92,6 @@ pub(crate) enum Instruction {
     /// Performs a function call with a list of its arguments.
     Call { func: ValueId, arguments: Vec<ValueId> },
 
-    /// Executes an "oracle" call
-    /// These are unconstrained functions that may access external state.
-    ForeignCall { func: String, arguments: Vec<ValueId> },
-
     /// Allocates a region of memory. Note that this is not concerned with
     /// the type of memory, the type of element is determined when loading this memory.
     /// This is used for representing mutable variables and references.
@@ -132,10 +128,9 @@ impl Instruction {
             }
             Instruction::ArraySet { array, .. } => InstructionResultType::Operand(*array),
             Instruction::Constrain(_) | Instruction::Store { .. } => InstructionResultType::None,
-            Instruction::Load { .. }
-            | Instruction::ArrayGet { .. }
-            | Instruction::Call { .. }
-            | Instruction::ForeignCall { .. } => InstructionResultType::Unknown,
+            Instruction::Load { .. } | Instruction::ArrayGet { .. } | Instruction::Call { .. } => {
+                InstructionResultType::Unknown
+            }
         }
     }
 
@@ -163,10 +158,6 @@ impl Instruction {
                 max_bit_size: *max_bit_size,
             },
             Instruction::Constrain(value) => Instruction::Constrain(f(*value)),
-            Instruction::ForeignCall { func, arguments } => Instruction::ForeignCall {
-                func: func.to_owned(),
-                arguments: vecmap(arguments.iter().copied(), f),
-            },
             Instruction::Call { func, arguments } => Instruction::Call {
                 func: f(*func),
                 arguments: vecmap(arguments.iter().copied(), f),
@@ -261,11 +252,10 @@ impl Instruction {
                     None
                 }
             }
-            Instruction::Call { .. } => None,
-            Instruction::ForeignCall { .. } => None,
-            Instruction::Allocate { .. } => None,
-            Instruction::Load { .. } => None,
-            Instruction::Store { .. } => None,
+            Instruction::Call { .. }
+            | Instruction::Allocate { .. }
+            | Instruction::Load { .. }
+            | Instruction::Store { .. } => None,
         }
     }
 }

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/printer.rs
@@ -150,9 +150,6 @@ pub(crate) fn display_instruction(
         Instruction::Call { func, arguments } => {
             writeln!(f, "call {}({})", show(*func), value_list(function, arguments))
         }
-        Instruction::ForeignCall { func, arguments } => {
-            writeln!(f, "foreign call {}({})", func, value_list(function, arguments))
-        }
         Instruction::Allocate => writeln!(f, "allocate"),
         Instruction::Load { address } => writeln!(f, "load {}", show(*address)),
         Instruction::Store { address, value } => {

diff --git a/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs b/crates/noirc_evaluator/src/ssa_refactor/opt/inlining.rs
@@ -330,7 +330,7 @@ impl<'function> PerFunctionContext<'function> {
                 Instruction::Call { func, arguments } => match self.get_function(*func) {
                     Some(function) => match ssa.functions[&function].runtime() {
                         RuntimeType::Acir => self.inline_function(ssa, *id, function, arguments),
-                        RuntimeType::Brillig | RuntimeType::Oracle(_) => {
+                        RuntimeType::Brillig => {
                             self.context.failed_to_inline_a_call = true;
                             self.push_instruction(*id);
                         }

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs
@@ -240,18 +240,6 @@ impl FunctionBuilder {
         self.insert_instruction(Instruction::Call { func, arguments }, Some(result_types)).results()
     }
 
-    /// Insert a foreign call instruction at the end of the current block and return
-    /// the results of the call.
-    pub(crate) fn insert_foreign_call(
-        &mut self,
-        func: String,
-        arguments: Vec<ValueId>,
-        result_types: Vec<Type>,
-    ) -> &[ValueId] {
-        self.insert_instruction(Instruction::ForeignCall { func, arguments }, Some(result_types))
-            .results()
-    }
-
     /// Insert an instruction to extract an element from an array
     pub(crate) fn insert_array_get(
         &mut self,

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs
@@ -279,25 +279,6 @@ impl<'a> FunctionContext<'a> {
         reshaped_return_values
     }
 
-    pub(super) fn insert_foreign_call(
-        &mut self,
-        function: String,
-        arguments: Vec<ValueId>,
-        result_type: &ast::Type,
-    ) -> Values {
-        let result_types = Self::convert_type(result_type).flatten();
-        let results = self.builder.insert_foreign_call(function, arguments, result_types);
-
-        let mut i = 0;
-        let reshaped_return_values = Self::map_type(result_type, |_| {
-            let result = results[i].into();
-            i += 1;
-            result
-        });
-        assert_eq!(i, results.len());
-        reshaped_return_values
-    }
-
     /// Create a const offset of an address for an array load or store
     pub(super) fn make_offset(&mut self, mut address: ValueId, offset: u128) -> ValueId {
         if offset != 0 {

diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/mod.rs
@@ -345,12 +345,6 @@ impl<'a> FunctionContext<'a> {
             .flat_map(|argument| self.codegen_expression(argument).into_value_list(self))
             .collect();
 
-        if let ast::Expression::Ident(ident) = call.func.as_ref() {
-            if let ast::Definition::Oracle(func) = &ident.definition {
-                return self.insert_foreign_call(func.to_owned(), arguments, &call.return_type);
-            }
-        }
-
         let function = self.codegen_non_tuple_expression(&call.func);
         self.insert_call(function, arguments, &call.return_type)
     }