Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: test PR merging #6585 and #6577 #6607

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5e04067
Avoid incrementing reference counts in some cases
jfecher Nov 20, 2024
d2a7849
Move inc_rc 2 instructions earlier
jfecher Nov 20, 2024
65054d6
Trigger CI
jfecher Nov 20, 2024
3e7d5f3
Merge branch 'master' into jf/rearrange-inc-rcs
jfecher Nov 20, 2024
97cf674
Trigger CI, remove external repos tag since noir-edwards needs an update
jfecher Nov 20, 2024
1b43b06
Merge branch 'jf/rearrange-inc-rcs' of https://github.com/noir-lang/n…
jfecher Nov 20, 2024
4c234cf
Update compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs
jfecher Nov 20, 2024
b4f0187
Remove need for extra parameter
jfecher Nov 21, 2024
204de4a
Start rc at 0, not 1
jfecher Nov 21, 2024
1e12188
added argument for initial rc
sirasistant Nov 21, 2024
6a4be88
Don't remove necessary RC instructions
jfecher Nov 21, 2024
a750a7a
Disable rc_tracker
jfecher Nov 21, 2024
a68d63f
Merge branch 'master' into jf/fix-die-rc
jfecher Nov 21, 2024
6543754
Narrow down issue to non_mutated_arrays tracking
jfecher Nov 22, 2024
9aaad7f
Merge branch 'jf/fix-die-rc' of https://github.com/noir-lang/noir int…
jfecher Nov 22, 2024
4b3950a
Disable test
jfecher Nov 22, 2024
e11afa0
Merge branch 'master' of https://github.com/noir-lang/noir into jf/fi…
jfecher Nov 22, 2024
602e1e6
Reintroduce optimization; it is per type now
jfecher Nov 22, 2024
fdc8dd0
Consider arrays returned by intrinsics to be fresh as well
jfecher Nov 22, 2024
e9b1ea9
Merge branch 'jf/init-rc-to-zero' into jf/fix-die-rc2
jfecher Nov 22, 2024
b36d8d3
Don't dedup arrays in brillig
jfecher Dec 2, 2024
c8b0ec9
Remove printlns
jfecher Dec 2, 2024
745c9d5
Merge branch 'master' into jf/fix-die-rc2
jfecher Dec 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,7 @@ impl<'block> BrilligBlock<'block> {
destination_vector,
source_size_register,
None,
None,
);

// Items
Expand Down Expand Up @@ -813,13 +814,14 @@ impl<'block> BrilligBlock<'block> {
// Initialize the variable
match new_variable {
BrilligVariable::BrilligArray(brillig_array) => {
self.brillig_context.codegen_initialize_array(brillig_array);
self.brillig_context.codegen_initialize_array(brillig_array, None);
}
BrilligVariable::BrilligVector(vector) => {
let size = self
.brillig_context
.make_usize_constant_instruction(array.len().into());
self.brillig_context.codegen_initialize_vector(vector, size, None);
self.brillig_context
.codegen_initialize_vector(vector, size, None, None);
self.brillig_context.deallocate_single_addr(size);
}
_ => unreachable!(
Expand Down Expand Up @@ -1814,7 +1816,7 @@ impl<'block> BrilligBlock<'block> {
unreachable!("ICE: allocate_foreign_call_array() expects an array, got {typ:?}")
};

self.brillig_context.codegen_initialize_array(array);
self.brillig_context.codegen_initialize_array(array, None);

let mut index = 0_usize;
for _ in 0..*size {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<
assert!(source_field.bit_size == F::max_num_bits());
assert!(radix.bit_size == 32);

self.codegen_initialize_array(target_array);
self.codegen_initialize_array(target_array, None);

let heap_array = self.codegen_brillig_array_to_heap_array(target_array);

Expand Down
18 changes: 13 additions & 5 deletions compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ use super::{
BrilligContext, ReservedRegisters, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
};

const INITIAL_ARRAY_REF_COUNT: usize = 0;

impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<F, Registers> {
/// Allocates an array of size `size` and stores the pointer to the array
/// in `pointer_register`
Expand Down Expand Up @@ -419,12 +421,16 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<
}

/// Initializes an array, allocating memory to store its representation and initializing the reference counter.
pub(crate) fn codegen_initialize_array(&mut self, array: BrilligArray) {
pub(crate) fn codegen_initialize_array(
&mut self,
array: BrilligArray,
initial_rc: Option<usize>,
) {
self.codegen_allocate_immediate_mem(array.pointer, array.size + 1);
self.indirect_const_instruction(
array.pointer,
BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
1_usize.into(),
initial_rc.unwrap_or(INITIAL_ARRAY_REF_COUNT).into(),
);
}

Expand All @@ -433,12 +439,13 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<
vector: BrilligVector,
size: SingleAddrVariable,
capacity: Option<SingleAddrVariable>,
initial_rc: Option<usize>,
) {
// Write RC
self.indirect_const_instruction(
vector.pointer,
BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
1_usize.into(),
initial_rc.unwrap_or(INITIAL_ARRAY_REF_COUNT).into(),
);

// Write size
Expand All @@ -459,6 +466,7 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<
vector: BrilligVector,
size: SingleAddrVariable,
capacity: Option<SingleAddrVariable>, // Defaults to size if None
initial_rc: Option<usize>, // Defaults to INITIAL_ARRAY_REF_COUNT if none
) {
let allocation_size = self.allocate_register();
// Allocation size = capacity + 3 (rc, size, capacity)
Expand All @@ -471,7 +479,7 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<
self.codegen_allocate_mem(vector.pointer, allocation_size);
self.deallocate_register(allocation_size);

self.codegen_initialize_vector_metadata(vector, size, capacity);
self.codegen_initialize_vector_metadata(vector, size, capacity, initial_rc);
}

/// We don't know the length of a vector returned externally before the call
Expand All @@ -498,7 +506,7 @@ impl<F: AcirField + DebugToString, Registers: RegisterAllocator> BrilligContext<
self.indirect_const_instruction(
vector.pointer,
BRILLIG_MEMORY_ADDRESSING_BIT_SIZE,
1_usize.into(),
INITIAL_ARRAY_REF_COUNT.into(),
);

// Initialize size
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,15 +199,15 @@ impl<F: AcirField + DebugToString> BrilligContext<F, Stack> {
let deflattened_items_pointer = if is_vector {
let vector = BrilligVector { pointer: deflattened_array_pointer };

self.codegen_initialize_vector(vector, deflattened_size_variable, None);
self.codegen_initialize_vector(vector, deflattened_size_variable, None, Some(1));

self.codegen_make_vector_items_pointer(vector)
} else {
let arr = BrilligArray {
pointer: deflattened_array_pointer,
size: item_count * item_type.len(),
};
self.codegen_initialize_array(arr);
self.codegen_initialize_array(arr, Some(1));
self.codegen_make_array_items_pointer(arr)
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ pub(crate) fn reallocate_vector_for_insertion<
target_vector,
target_size,
Some(source_capacity),
Some(1),
);
}
});
Expand All @@ -201,6 +202,7 @@ pub(crate) fn reallocate_vector_for_insertion<
target_vector,
target_size,
Some(double_size),
Some(1),
);
brillig_context.deallocate_single_addr(double_size);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ pub(super) fn compile_vector_pop_back_procedure<F: AcirField + DebugToString>(
brillig_context.codegen_update_vector_length(target_vector, target_size);
} else {
// We need to clone the source vector
brillig_context.codegen_initialize_vector(target_vector, target_size, None);
brillig_context.codegen_initialize_vector(target_vector, target_size, None, Some(1));

let target_vector_items_pointer =
brillig_context.codegen_make_vector_items_pointer(target_vector);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,10 @@ pub(super) fn compile_vector_pop_front_procedure<F: AcirField + DebugToString>(
target_vector,
target_size,
Some(source_capacity),
Some(1),
);
} else {
brillig_context.codegen_initialize_vector(target_vector, target_size, None);
brillig_context.codegen_initialize_vector(target_vector, target_size, None, Some(1));

let target_vector_items_pointer =
brillig_context.codegen_make_vector_items_pointer(target_vector);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ pub(super) fn compile_vector_remove_procedure<F: AcirField + DebugToString>(
brillig_context
.codegen_vector_items_pointer(target_vector, target_vector_items_pointer);
} else {
brillig_context.codegen_initialize_vector(target_vector, target_size, None);
brillig_context.codegen_initialize_vector(target_vector, target_size, None, Some(1));

// Copy the elements to the left of the index
brillig_context
Expand Down
3 changes: 2 additions & 1 deletion compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,8 @@ impl<'f> FunctionInserter<'f> {
// another MakeArray instruction. Note that this assumes the function inserter is inserting
// in control-flow order. Otherwise we could refer to ValueIds defined later in the program.
let make_array = if let Instruction::MakeArray { elements, typ } = &instruction {
if self.array_is_constant(elements) {
// Array caching is disabled for brillig functions since they may be mutated directly
if self.array_is_constant(elements) && self.function.runtime().is_acir() {
if let Some(fetched_value) = self.get_cached_array(elements, typ) {
assert_eq!(results.len(), 1);
self.values.insert(results[0], fetched_value);
Expand Down
10 changes: 5 additions & 5 deletions compiler/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ impl Instruction {
/// conditional on whether the caller wants the predicate to be taken into account or not.
pub(crate) fn can_be_deduplicated(
&self,
dfg: &DataFlowGraph,
function: &Function,
deduplicate_with_predicate: bool,
) -> bool {
use Instruction::*;
Expand All @@ -438,7 +438,7 @@ impl Instruction {
| IncrementRc { .. }
| DecrementRc { .. } => false,

Call { func, .. } => match dfg[*func] {
Call { func, .. } => match function.dfg[*func] {
Value::Intrinsic(intrinsic) => {
intrinsic.can_be_deduplicated(deduplicate_with_predicate)
}
Expand All @@ -448,8 +448,8 @@ impl Instruction {
// We can deduplicate these instructions if we know the predicate is also the same.
Constrain(..) | RangeCheck { .. } => deduplicate_with_predicate,

// This should never be side-effectful
MakeArray { .. } => true,
// Arrays can be mutated in unconstrained code so we can't deduplicate there
MakeArray { .. } => function.runtime().is_acir(),

// These can have different behavior depending on the EnableSideEffectsIf context.
// Replacing them with a similar instruction potentially enables replacing an instruction
Expand All @@ -462,7 +462,7 @@ impl Instruction {
| IfElse { .. }
| ArrayGet { .. }
| ArraySet { .. } => {
deduplicate_with_predicate || !self.requires_acir_gen_predicate(dfg)
deduplicate_with_predicate || !self.requires_acir_gen_predicate(&function.dfg)
}
}
}
Expand Down
48 changes: 27 additions & 21 deletions compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ impl Function {
}

context.visited_blocks.insert(block);
context.fold_constants_in_block(&mut self.dfg, &mut dom, block);
context.fold_constants_in_block(self, &mut dom, block);
}
}
}
Expand Down Expand Up @@ -266,49 +266,55 @@ impl<'brillig> Context<'brillig> {

fn fold_constants_in_block(
&mut self,
dfg: &mut DataFlowGraph,
function: &mut Function,
dom: &mut DominatorTree,
block: BasicBlockId,
) {
let instructions = dfg[block].take_instructions();
let instructions = function.dfg[block].take_instructions();

// Default side effect condition variable with an enabled state.
let mut side_effects_enabled_var = dfg.make_constant(FieldElement::one(), Type::bool());
let mut side_effects_enabled_var =
function.dfg.make_constant(FieldElement::one(), Type::bool());

for instruction_id in instructions {
self.fold_constants_into_instruction(
dfg,
function,
dom,
block,
instruction_id,
&mut side_effects_enabled_var,
);
}
self.block_queue.extend(dfg[block].successors());
self.block_queue.extend(function.dfg[block].successors());
}

fn fold_constants_into_instruction(
&mut self,
dfg: &mut DataFlowGraph,
function: &mut Function,
dom: &mut DominatorTree,
mut block: BasicBlockId,
id: InstructionId,
side_effects_enabled_var: &mut ValueId,
) {
let constraint_simplification_mapping = self.get_constraint_map(*side_effects_enabled_var);

let instruction =
Self::resolve_instruction(id, block, dfg, dom, constraint_simplification_mapping);
let instruction = Self::resolve_instruction(
id,
block,
&function.dfg,
dom,
constraint_simplification_mapping,
);

let old_results = dfg.instruction_results(id).to_vec();
let old_results = function.dfg.instruction_results(id).to_vec();

// If a copy of this instruction exists earlier in the block, then reuse the previous results.
if let Some(cache_result) =
self.get_cached(dfg, dom, &instruction, *side_effects_enabled_var, block)
self.get_cached(&function.dfg, dom, &instruction, *side_effects_enabled_var, block)
{
match cache_result {
CacheResult::Cached(cached) => {
Self::replace_result_ids(dfg, &old_results, cached);
Self::replace_result_ids(&mut function.dfg, &old_results, cached);
return;
}
CacheResult::NeedToHoistToCommonBlock(dominator) => {
Expand All @@ -327,7 +333,7 @@ impl<'brillig> Context<'brillig> {
&instruction,
&old_results,
block,
dfg,
&mut function.dfg,
self.brillig_info,
)
.unwrap_or_else(|| {
Expand All @@ -337,16 +343,16 @@ impl<'brillig> Context<'brillig> {
instruction.clone(),
&old_results,
block,
dfg,
&mut function.dfg,
)
});

Self::replace_result_ids(dfg, &old_results, &new_results);
Self::replace_result_ids(&mut function.dfg, &old_results, &new_results);

self.cache_instruction(
instruction.clone(),
new_results,
dfg,
function,
*side_effects_enabled_var,
block,
);
Expand Down Expand Up @@ -433,7 +439,7 @@ impl<'brillig> Context<'brillig> {
&mut self,
instruction: Instruction,
instruction_results: Vec<ValueId>,
dfg: &DataFlowGraph,
function: &Function,
side_effects_enabled_var: ValueId,
block: BasicBlockId,
) {
Expand All @@ -442,21 +448,21 @@ impl<'brillig> Context<'brillig> {
// to map from the more complex to the simpler value.
if let Instruction::Constrain(lhs, rhs, _) = instruction {
// These `ValueId`s should be fully resolved now.
if let Some((complex, simple)) = simplify(dfg, lhs, rhs) {
if let Some((complex, simple)) = simplify(&function.dfg, lhs, rhs) {
self.get_constraint_map(side_effects_enabled_var)
.entry(complex)
.or_default()
.add(dfg, simple, block);
.add(&function.dfg, simple, block);
}
}
}

// If the instruction doesn't have side-effects and if it won't interact with enable_side_effects during acir_gen,
// we cache the results so we can reuse them if the same instruction appears again later in the block.
// Others have side effects representing failure, which are implicit in the ACIR code and can also be deduplicated.
if instruction.can_be_deduplicated(dfg, self.use_constraint_info) {
if instruction.can_be_deduplicated(function, self.use_constraint_info) {
let use_predicate =
self.use_constraint_info && instruction.requires_acir_gen_predicate(dfg);
self.use_constraint_info && instruction.requires_acir_gen_predicate(&function.dfg);
let predicate = use_predicate.then_some(side_effects_enabled_var);

self.cached_instruction_results
Expand Down
Loading
Loading