fix(ssa): don't deduplicate constraints in blocks that are not domina…

…ted (#6627)

Co-authored-by: Akosh Farkash <[email protected]>
Co-authored-by: Tom French <[email protected]>

compiler/noirc_evaluator/src/ssa/ir/dom.rs

@@ -119,6 +119,29 @@ impl DominatorTree {
         }
     }
 
+    /// Walk up the dominator tree until we find a block for which `f` returns `Some` value.
+    /// Otherwise return `None` when we reach the top.
+    ///
+    /// Similar to `Iterator::filter_map` but only returns the first hit.
+    pub(crate) fn find_map_dominator<T>(
+        &self,
+        mut block_id: BasicBlockId,
+        f: impl Fn(BasicBlockId) -> Option<T>,
+    ) -> Option<T> {
+        if !self.is_reachable(block_id) {
+            return None;
+        }
+        loop {
+            if let Some(value) = f(block_id) {
+                return Some(value);
+            }
+            block_id = match self.immediate_dominator(block_id) {
+                Some(immediate_dominator) => immediate_dominator,
+                None => return None,
+            }
+        }
+    }
+
     /// Allocate and compute a dominator tree from a pre-computed control flow graph and
     /// post-order counterpart.
     pub(crate) fn with_cfg_and_post_order(cfg: &ControlFlowGraph, post_order: &PostOrder) -> Self {
@@ -448,4 +471,22 @@ mod tests {
         assert!(dt.dominates(block2_id, block1_id));
         assert!(dt.dominates(block2_id, block2_id));
     }
+
+    #[test]
+    fn test_find_map_dominator() {
+        let (dt, b0, b1, b2, _b3) = unreachable_node_setup();
+
+        assert_eq!(
+            dt.find_map_dominator(b2, |b| if b == b0 { Some("root") } else { None }),
+            Some("root")
+        );
+        assert_eq!(
+            dt.find_map_dominator(b1, |b| if b == b0 { Some("unreachable") } else { None }),
+            None
+        );
+        assert_eq!(
+            dt.find_map_dominator(b1, |b| if b == b1 { Some("not part of tree") } else { None }),
+            None
+        );
+    }
 }

compiler/noirc_evaluator/src/ssa/ir/instruction.rs

@@ -362,6 +362,44 @@ impl Instruction {
         matches!(self.result_type(), InstructionResultType::Unknown)
     }
 
+    /// Indicates if the instruction has a side effect, ie. it can fail, or it interacts with memory.
+    ///
+    /// This is similar to `can_be_deduplicated`, but it doesn't depend on whether the caller takes
+    /// constraints into account, because it might not use it to isolate the side effects across branches.
+    pub(crate) fn has_side_effects(&self, dfg: &DataFlowGraph) -> bool {
+        use Instruction::*;
+
+        match self {
+            // These either have side-effects or interact with memory
+            EnableSideEffectsIf { .. }
+            | Allocate
+            | Load { .. }
+            | Store { .. }
+            | IncrementRc { .. }
+            | DecrementRc { .. } => true,
+
+            Call { func, .. } => match dfg[*func] {
+                Value::Intrinsic(intrinsic) => intrinsic.has_side_effects(),
+                _ => true, // Be conservative and assume other functions can have side effects.
+            },
+
+            // These can fail.
+            Constrain(..) | RangeCheck { .. } => true,
+
+            // This should never be side-effectful
+            MakeArray { .. } => false,
+
+            // These can have different behavior depending on the EnableSideEffectsIf context.
+            Binary(_)
+            | Cast(_, _)
+            | Not(_)
+            | Truncate { .. }
+            | IfElse { .. }
+            | ArrayGet { .. }
+            | ArraySet { .. } => self.requires_acir_gen_predicate(dfg),
+        }
+    }
+
     /// Indicates if the instruction can be safely replaced with the results of another instruction with the same inputs.
     /// If `deduplicate_with_predicate` is set, we assume we're deduplicating with the instruction
     /// and its predicate, rather than just the instruction. Setting this means instructions that