Skip to content

Commit b059fd1

Browse files
jfechervezenovm
jfecher
and
vezenovm
authored
fix: Experiment with DecrementRc (#7629)
Co-authored-by: Maxim Vezenov <mvezenov@gmail.com>
1 parent 318bd88 commit b059fd1

File tree

28 files changed

+413
-135
lines changed

28 files changed

+413
-135
lines changed

‎compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs

+24-50
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ use crate::ssa::ir::{
2020
value::{Value, ValueId},
2121
};
2222
use acvm::acir::brillig::{MemoryAddress, ValueOrArray};
23-
use acvm::brillig_vm::MEMORY_ADDRESSING_BIT_SIZE;
2423
use acvm::{FieldElement, acir::AcirField};
2524
use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet};
2625
use iter_extended::vecmap;
@@ -872,65 +871,40 @@ impl<'block, Registers: RegisterAllocator> BrilligBlock<'block, Registers> {
872871
.store_instruction(array_or_vector.extract_register(), rc_register);
873872
self.brillig_context.deallocate_register(rc_register);
874873
}
875-
Instruction::DecrementRc { value, original } => {
874+
Instruction::DecrementRc { value } => {
876875
let array_or_vector = self.convert_ssa_value(*value, dfg);
877-
let orig_array_or_vector = self.convert_ssa_value(*original, dfg);
878-
879876
let array_register = array_or_vector.extract_register();
880-
let orig_array_register = orig_array_or_vector.extract_register();
881-
882-
let codegen_dec_rc = |ctx: &mut BrilligContext<FieldElement, Registers>| {
883-
let rc_register = ctx.allocate_register();
884-
885-
ctx.load_instruction(rc_register, array_register);
886-
887-
ctx.codegen_usize_op_in_place(rc_register, BrilligBinaryOp::Sub, 1);
888-
889-
// Check that the refcount didn't go below 1. If we allow it to underflow
890-
// and become 0 or usize::MAX, and then return to 1, then it will indicate
891-
// an array as mutable when it probably shouldn't be.
892-
if ctx.enable_debug_assertions() {
893-
let min_addr = ReservedRegisters::usize_one();
894-
let condition = SingleAddrVariable::new(ctx.allocate_register(), 1);
895-
ctx.memory_op_instruction(
896-
min_addr,
897-
rc_register,
898-
condition.address,
899-
BrilligBinaryOp::LessThanEquals,
900-
);
901-
ctx.codegen_constrain(
902-
condition,
903-
Some("array ref-count went to zero".to_owned()),
904-
);
905-
ctx.deallocate_single_addr(condition);
906-
}
907877

908-
ctx.store_instruction(array_register, rc_register);
909-
ctx.deallocate_register(rc_register);
910-
};
878+
let rc_register = self.brillig_context.allocate_register();
879+
self.brillig_context.load_instruction(rc_register, array_register);
911880

912-
if array_register == orig_array_register {
913-
codegen_dec_rc(self.brillig_context);
914-
} else {
915-
// Check if the current and original register point at the same memory.
916-
// If they don't, it means that an array-copy procedure has created a
917-
// new array, which includes decrementing the RC of the original,
918-
// which means we don't have to do the decrement here.
881+
// Check that the refcount isn't already 0 before we decrement. If we allow it to underflow
882+
// and become usize::MAX, and then return to 1, then it will indicate
883+
// an array as mutable when it probably shouldn't be.
884+
if self.brillig_context.enable_debug_assertions() {
885+
let min_addr = ReservedRegisters::usize_one();
919886
let condition =
920887
SingleAddrVariable::new(self.brillig_context.allocate_register(), 1);
921-
// We will use a binary op to interpret the pointer as a number.
922-
let bit_size: u32 = MEMORY_ADDRESSING_BIT_SIZE.into();
923-
let array_var = SingleAddrVariable::new(array_register, bit_size);
924-
let orig_array_var = SingleAddrVariable::new(orig_array_register, bit_size);
925-
self.brillig_context.binary_instruction(
926-
array_var,
927-
orig_array_var,
888+
self.brillig_context.memory_op_instruction(
889+
min_addr,
890+
rc_register,
891+
condition.address,
892+
BrilligBinaryOp::LessThan,
893+
);
894+
self.brillig_context.codegen_constrain(
928895
condition,
929-
BrilligBinaryOp::Equals,
896+
Some("array ref-count underflow detected".to_owned()),
930897
);
931-
self.brillig_context.codegen_if(condition.address, codegen_dec_rc);
932898
self.brillig_context.deallocate_single_addr(condition);
933899
}
900+
901+
self.brillig_context.codegen_usize_op_in_place(
902+
rc_register,
903+
BrilligBinaryOp::Sub,
904+
1,
905+
);
906+
self.brillig_context.store_instruction(array_register, rc_register);
907+
self.brillig_context.deallocate_register(rc_register);
934908
}
935909
Instruction::EnableSideEffectsIf { .. } => {
936910
unreachable!("enable_side_effects not supported by brillig")

‎compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -1056,8 +1056,8 @@ mod test {
10561056
v19 = call f1(v5) -> u32
10571057
v20 = add v8, v19
10581058
constrain v6 == v20
1059-
dec_rc v4 v4
1060-
dec_rc v5 v5
1059+
dec_rc v4
1060+
dec_rc v5
10611061
return
10621062
}
10631063

‎compiler/noirc_evaluator/src/ssa/function_builder/mod.rs

+10-18
Original file line numberDiff line numberDiff line change
@@ -394,8 +394,8 @@ impl FunctionBuilder {
394394

395395
/// Insert an instruction to decrement an array's reference count. This only has an effect
396396
/// in unconstrained code where arrays are reference counted and copy on write.
397-
pub(crate) fn insert_dec_rc(&mut self, value: ValueId, original: ValueId) {
398-
self.insert_instruction(Instruction::DecrementRc { value, original }, None);
397+
pub(crate) fn insert_dec_rc(&mut self, value: ValueId) {
398+
self.insert_instruction(Instruction::DecrementRc { value }, None);
399399
}
400400

401401
/// Insert an enable_side_effects_if instruction. These are normally only automatically
@@ -497,20 +497,16 @@ impl FunctionBuilder {
497497
///
498498
/// Returns the ID of the array that was affected, if any.
499499
pub(crate) fn increment_array_reference_count(&mut self, value: ValueId) -> Option<ValueId> {
500-
self.update_array_reference_count(value, None)
500+
self.update_array_reference_count(value, true)
501501
}
502502

503503
/// Insert instructions to decrement the reference count of any array(s) stored
504504
/// within the given value. If the given value is not an array and does not contain
505505
/// any arrays, this does nothing.
506506
///
507507
/// Returns the ID of the array that was affected, if any.
508-
pub(crate) fn decrement_array_reference_count(
509-
&mut self,
510-
value: ValueId,
511-
original: ValueId,
512-
) -> Option<ValueId> {
513-
self.update_array_reference_count(value, Some(original))
508+
pub(crate) fn decrement_array_reference_count(&mut self, value: ValueId) -> Option<ValueId> {
509+
self.update_array_reference_count(value, false)
514510
}
515511

516512
/// Increment or decrement the given value's reference count if it is an array.
@@ -522,11 +518,7 @@ impl FunctionBuilder {
522518
/// the meantime, which in itself would make sure its RC is decremented.
523519
///
524520
/// Returns the ID of the array that was affected, if any.
525-
fn update_array_reference_count(
526-
&mut self,
527-
value: ValueId,
528-
original: Option<ValueId>,
529-
) -> Option<ValueId> {
521+
fn update_array_reference_count(&mut self, value: ValueId, increment: bool) -> Option<ValueId> {
530522
if self.current_function.runtime().is_acir() {
531523
return None;
532524
}
@@ -536,7 +528,7 @@ impl FunctionBuilder {
536528
if element.contains_an_array() {
537529
let reference = value;
538530
let value = self.insert_load(reference, element.as_ref().clone());
539-
self.update_array_reference_count(value, original);
531+
self.update_array_reference_count(value, increment);
540532
Some(value)
541533
} else {
542534
None
@@ -545,10 +537,10 @@ impl FunctionBuilder {
545537
Type::Array(..) | Type::Slice(..) => {
546538
// If there are nested arrays or slices, we wait until ArrayGet
547539
// is issued to increment the count of that array.
548-
if let Some(original) = original {
549-
self.insert_dec_rc(value, original);
550-
} else {
540+
if increment {
551541
self.insert_inc_rc(value);
542+
} else {
543+
self.insert_dec_rc(value);
552544
}
553545
Some(value)
554546
}

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

+6-13
Original file line numberDiff line numberDiff line change
@@ -325,9 +325,7 @@ pub(crate) enum Instruction {
325325
/// This currently only has an effect in Brillig code where array sharing and copy on write is
326326
/// implemented via reference counting. In ACIR code this is done with im::Vector and these
327327
/// DecrementRc instructions are ignored.
328-
///
329-
/// The `original` contains the value of the array which was incremented by the pair of this decrement.
330-
DecrementRc { value: ValueId, original: ValueId },
328+
DecrementRc { value: ValueId },
331329

332330
/// Merge two values returned from opposite branches of a conditional into one.
333331
///
@@ -724,9 +722,7 @@ impl Instruction {
724722
mutable: *mutable,
725723
},
726724
Instruction::IncrementRc { value } => Instruction::IncrementRc { value: f(*value) },
727-
Instruction::DecrementRc { value, original } => {
728-
Instruction::DecrementRc { value: f(*value), original: f(*original) }
729-
}
725+
Instruction::DecrementRc { value } => Instruction::DecrementRc { value: f(*value) },
730726
Instruction::RangeCheck { value, max_bit_size, assert_message } => {
731727
Instruction::RangeCheck {
732728
value: f(*value),
@@ -797,9 +793,8 @@ impl Instruction {
797793
*value = f(*value);
798794
}
799795
Instruction::IncrementRc { value } => *value = f(*value),
800-
Instruction::DecrementRc { value, original } => {
796+
Instruction::DecrementRc { value } => {
801797
*value = f(*value);
802-
*original = f(*original);
803798
}
804799
Instruction::RangeCheck { value, max_bit_size: _, assert_message: _ } => {
805800
*value = f(*value);
@@ -866,12 +861,10 @@ impl Instruction {
866861
Instruction::EnableSideEffectsIf { condition } => {
867862
f(*condition);
868863
}
869-
Instruction::IncrementRc { value } | Instruction::RangeCheck { value, .. } => {
870-
f(*value);
871-
}
872-
Instruction::DecrementRc { value, original } => {
864+
Instruction::IncrementRc { value }
865+
| Instruction::DecrementRc { value }
866+
| Instruction::RangeCheck { value, .. } => {
873867
f(*value);
874-
f(*original);
875868
}
876869
Instruction::IfElse { then_condition, then_value, else_condition, else_value } => {
877870
f(*then_condition);

‎compiler/noirc_evaluator/src/ssa/ir/printer.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -250,8 +250,8 @@ fn display_instruction_inner(
250250
Instruction::IncrementRc { value } => {
251251
writeln!(f, "inc_rc {}", show(*value))
252252
}
253-
Instruction::DecrementRc { value, original } => {
254-
writeln!(f, "dec_rc {} {}", show(*value), show(*original))
253+
Instruction::DecrementRc { value } => {
254+
writeln!(f, "dec_rc {}", show(*value))
255255
}
256256
Instruction::RangeCheck { value, max_bit_size, .. } => {
257257
writeln!(f, "range_check {} to {} bits", show(*value), *max_bit_size,)

‎compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -1445,7 +1445,7 @@ mod test {
14451445
v2 = array_get v0, index u32 0 -> Field
14461446
v4 = array_get v0, index u32 1 -> Field
14471447
v5 = add v2, v4
1448-
dec_rc v0 v0
1448+
dec_rc v0
14491449
return v5
14501450
}
14511451
";

‎compiler/noirc_evaluator/src/ssa/opt/die.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -838,7 +838,7 @@ mod test {
838838
b0(v0: [Field; 2]):
839839
inc_rc v0
840840
v2 = array_get v0, index u32 0 -> Field
841-
dec_rc v0 v0
841+
dec_rc v0
842842
return v2
843843
}
844844
";
@@ -862,7 +862,7 @@ mod test {
862862
b0(v0: [Field; 2]):
863863
inc_rc v0
864864
v2 = array_set v0, index u32 0, value u32 0
865-
dec_rc v0 v0
865+
dec_rc v0
866866
return v2
867867
}
868868
";
@@ -970,7 +970,7 @@ mod test {
970970
v3 = load v0 -> [Field; 3]
971971
v6 = array_set v3, index u32 0, value Field 5
972972
store v6 at v0
973-
dec_rc v6 v1
973+
dec_rc v6
974974
return
975975
}
976976
";

‎compiler/noirc_evaluator/src/ssa/opt/rc.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ mod test {
190190
b0(v0: [Field; 2]):
191191
inc_rc v0
192192
inc_rc v0
193-
dec_rc v0 v0
193+
dec_rc v0
194194
v1 = make_array [v0] : [[Field; 2]; 1]
195195
return v1
196196
}
@@ -218,7 +218,7 @@ mod test {
218218
v2 = load v1 -> [Field; 2]
219219
v5 = array_set v2, index u64 0, value Field 5
220220
store v5 at v1
221-
dec_rc v0 v0
221+
dec_rc v0
222222
return
223223
}
224224
";
@@ -250,7 +250,7 @@ mod test {
250250
v5 = array_set v2, index u64 0, value Field 5
251251
store v5 at v0
252252
v6 = load v0 -> [Field; 2]
253-
dec_rc v6 v1
253+
dec_rc v1
254254
store v6 at v0
255255
return
256256
}

‎compiler/noirc_evaluator/src/ssa/opt/unrolling.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -1287,7 +1287,7 @@ mod tests {
12871287
jmp b1()
12881288
b1():
12891289
v20 = load v3 -> [u64; 6]
1290-
dec_rc v0 v0
1290+
dec_rc v0
12911291
return v20
12921292
}
12931293
";
@@ -1463,7 +1463,7 @@ mod tests {
14631463
jmp b1(v16)
14641464
b2():
14651465
v8 = load v4 -> [u64; 6]
1466-
dec_rc v0 v0
1466+
dec_rc v0
14671467
return v8
14681468
}}
14691469
"

‎compiler/noirc_evaluator/src/ssa/parser/ast.rs

-1
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,6 @@ pub(crate) enum ParsedInstruction {
116116
},
117117
DecrementRc {
118118
value: ParsedValue,
119-
original: ParsedValue,
120119
},
121120
EnableSideEffectsIf {
122121
condition: ParsedValue,

‎compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs

+2-3
Original file line numberDiff line numberDiff line change
@@ -284,10 +284,9 @@ impl Translator {
284284
};
285285
self.builder.insert_constrain(lhs, rhs, assert_message);
286286
}
287-
ParsedInstruction::DecrementRc { value, original } => {
287+
ParsedInstruction::DecrementRc { value } => {
288288
let value = self.translate_value(value)?;
289-
let original = self.translate_value(original)?;
290-
self.builder.decrement_array_reference_count(value, original);
289+
self.builder.decrement_array_reference_count(value);
291290
}
292291
ParsedInstruction::EnableSideEffectsIf { condition } => {
293292
let condition = self.translate_value(condition)?;

‎compiler/noirc_evaluator/src/ssa/parser/mod.rs

+1-2
Original file line numberDiff line numberDiff line change
@@ -407,8 +407,7 @@ impl<'a> Parser<'a> {
407407
}
408408

409409
let value = self.parse_value_or_error()?;
410-
let original = self.parse_value_or_error()?;
411-
Ok(Some(ParsedInstruction::DecrementRc { value, original }))
410+
Ok(Some(ParsedInstruction::DecrementRc { value }))
412411
}
413412

414413
fn parse_enable_side_effects(&mut self) -> ParseResult<Option<ParsedInstruction>> {

‎compiler/noirc_evaluator/src/ssa/parser/tests.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -474,7 +474,7 @@ fn test_dec_rc() {
474474
let src = "
475475
brillig(inline) fn main f0 {
476476
b0(v0: [Field; 3]):
477-
dec_rc v0 v0
477+
dec_rc v0
478478
return
479479
}
480480
";

‎compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs

+5-1
Original file line numberDiff line numberDiff line change
@@ -1001,11 +1001,15 @@ impl<'a> FunctionContext<'a> {
10011001
mut incremented_params: Vec<(ValueId, ValueId)>,
10021002
terminator_args: &[ValueId],
10031003
) {
1004+
// TODO: This check likely leads to unsoundness.
1005+
// It is here to avoid decrementing the RC of a parameter we're returning but we
1006+
// only check the exact ValueId which can be easily circumvented by storing to and
1007+
// loading from a temporary reference.
10041008
incremented_params.retain(|(parameter, _)| !terminator_args.contains(parameter));
10051009

10061010
for (parameter, original) in incremented_params {
10071011
if self.builder.current_function.dfg.value_is_reference(parameter) {
1008-
self.builder.decrement_array_reference_count(parameter, original);
1012+
self.builder.decrement_array_reference_count(original);
10091013
}
10101014
}
10111015
}

‎test_programs/execution_success/reference_counts/Nargo.toml ‎test_programs/execution_success/reference_counts_inliner_0/Nargo.toml

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
[package]
2-
name = "reference_counts"
2+
name = "reference_counts_inliner_0"
33
type = "bin"
44
authors = [""]
55
compiler_version = ">=0.35.0"

‎test_programs/execution_success/reference_counts/Prover.toml ‎test_programs/execution_success/reference_counts_inliner_0/Prover.toml

File renamed without changes.

0 commit comments

Comments
 (0)