chore: disallow inserting ACIR-only instructions into brillig functio…

…ns (#7017)
noir-lang · Jan 10, 2025 · 9fcaed8 · 9fcaed8
1 parent f6ed6aa
commit 9fcaed8
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 11 deletions.
diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs
@@ -795,7 +795,7 @@ impl<'block> BrilligBlock<'block> {
                 self.brillig_context.deallocate_register(rc_register);
             }
             Instruction::EnableSideEffectsIf { .. } => {
-                todo!("enable_side_effects not supported by brillig")
+                unreachable!("enable_side_effects not supported by brillig")
             }
             Instruction::IfElse { .. } => {
                 unreachable!("IfElse instructions should not be possible in brillig")

diff --git a/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/compiler/noirc_evaluator/src/ssa/ir/dfg.rs
@@ -20,6 +20,7 @@ use iter_extended::vecmap;
 use serde::{Deserialize, Serialize};
 use serde_with::serde_as;
 use serde_with::DisplayFromStr;
+use tracing::warn;
 
 /// The DataFlowGraph contains most of the actual data in a function including
 /// its blocks, instructions, and values. This struct is largely responsible for
@@ -181,7 +182,16 @@ impl DataFlowGraph {
 
     /// Check if the function runtime would simply ignore this instruction.
     pub(crate) fn is_handled_by_runtime(&self, instruction: &Instruction) -> bool {
-        !(self.runtime().is_acir() && instruction.is_brillig_only())
+        match self.runtime() {
+            RuntimeType::Acir(_) => !matches!(
+                instruction,
+                Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. }
+            ),
+            RuntimeType::Brillig(_) => !matches!(
+                instruction,
+                Instruction::EnableSideEffectsIf { .. } | Instruction::IfElse { .. }
+            ),
+        }
     }
 
     fn insert_instruction_without_simplification(
@@ -205,7 +215,7 @@ impl DataFlowGraph {
         call_stack: CallStackId,
     ) -> InsertInstructionResult {
         if !self.is_handled_by_runtime(&instruction_data) {
-            return InsertInstructionResult::InstructionRemoved;
+            panic!("Attempted to insert instruction not handled by runtime: {instruction_data:?}");
         }
 
         let id = self.insert_instruction_without_simplification(
@@ -228,6 +238,7 @@ impl DataFlowGraph {
         call_stack: CallStackId,
     ) -> InsertInstructionResult {
         if !self.is_handled_by_runtime(&instruction) {
+            warn!("Attempted to insert instruction not handled by runtime: {instruction:?}");
             return InsertInstructionResult::InstructionRemoved;
         }
 

diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction.rs
@@ -1066,12 +1066,6 @@ impl Instruction {
             Instruction::Noop => Remove,
         }
     }
-
-    /// Some instructions are only to be used in Brillig and should be eliminated
-    /// after runtime separation, never to be be reintroduced in an ACIR runtime.
-    pub(crate) fn is_brillig_only(&self) -> bool {
-        matches!(self, Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. })
-    }
 }
 
 /// Given a chain of operations like:

diff --git a/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs
@@ -1565,7 +1565,6 @@ mod test {
             v10 = mul v0, v9                                 // attaching `c` to `a`
             v11 = call to_be_radix(v10, u32 256) -> [u8; 1]  // calling `to_radix(c * a)`
             inc_rc v11
-            enable_side_effects v2                           // side effect var for `c` shifted down by removal
             return
         }
         ";
@@ -1580,7 +1579,6 @@ mod test {
             v7 = mul v0, v6
             v8 = call to_be_radix(v7, u32 256) -> [u8; 1]
             inc_rc v8
-            enable_side_effects v2
             return
         }
         ";