From 6a4be880bdd1d8dd8a0f9850e372882b4620fb82 Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 21 Nov 2024 11:52:12 -0600 Subject: [PATCH 1/3] Don't remove necessary RC instructions --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 8d3fa9cc615..0460b29753a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -127,8 +127,9 @@ impl Context { .push(instructions_len - instruction_index - 1); } } else { - use Instruction::*; - if matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) { + // We can't remove rc instructions if they're loaded from a reference + // since we'd have no way of knowing whether the reference is still used. + if Self::is_inc_dec_instruction_on_known_array(instruction, &function.dfg) { self.rc_instructions.push((*instruction_id, block_id)); } else { instruction.for_each_value(|value| { @@ -337,6 +338,21 @@ impl Context { inserted_check } + + /// True if this is a `Instruction::IncrementRc` or `Instruction::DecrementRc` + /// operating on an array directly from a `Instruction::MakeArray`. + fn is_inc_dec_instruction_on_known_array( + instruction: &Instruction, + dfg: &DataFlowGraph, + ) -> bool { + use Instruction::*; + if let IncrementRc { value } | DecrementRc { value } = instruction { + if let Value::Instruction { instruction, .. } = &dfg[*value] { + return matches!(&dfg[*instruction], MakeArray { .. }); + } + } + false + } } fn instruction_might_result_in_out_of_bounds( From a750a7aba3fa8dfea0d9e30b61a2140cf3baf41e Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Thu, 21 Nov 2024 12:01:14 -0600 Subject: [PATCH 2/3] Disable rc_tracker --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 0460b29753a..584e852e01b 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -108,7 +108,7 @@ impl Context { let instructions_len = block.instructions().len(); - let mut rc_tracker = RcTracker::default(); + // let mut rc_tracker = RcTracker::default(); // Indexes of instructions that might be out of bounds. // We'll remove those, but before that we'll insert bounds checks for them. @@ -138,11 +138,11 @@ impl Context { } } - rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); + // rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); } - self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays()); - self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); + // self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays()); + // self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); // If there are some instructions that might trigger an out of bounds error, // first add constrain checks. Then run the DIE pass again, which will remove those From 6543754566177a5c441dcb912893d8a660e51d0b Mon Sep 17 00:00:00 2001 From: Jake Fecher Date: Fri, 22 Nov 2024 10:26:15 -0600 Subject: [PATCH 3/3] Narrow down issue to non_mutated_arrays tracking --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index 584e852e01b..3d1be5578ae 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -108,7 +108,7 @@ impl Context { let instructions_len = block.instructions().len(); - // let mut rc_tracker = RcTracker::default(); + let mut rc_tracker = RcTracker::default(); // Indexes of instructions that might be out of bounds. // We'll remove those, but before that we'll insert bounds checks for them. @@ -138,11 +138,11 @@ impl Context { } } - // rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); + rc_tracker.track_inc_rcs_to_remove(*instruction_id, function); } // self.instructions_to_remove.extend(rc_tracker.get_non_mutated_arrays()); - // self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); + self.instructions_to_remove.extend(rc_tracker.rc_pairs_to_remove); // If there are some instructions that might trigger an out of bounds error, // first add constrain checks. Then run the DIE pass again, which will remove those