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.

Sign up for GitHub

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

Jump to bottom

fix(ssa): Change array_set to not mutate slices coming from function inputs #6463

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(ssa): Change array_set to not mutate slices coming from function inputs #6463

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9685e8b
Fix SSA array_set to not mutate arrays/slices which are input to the …
aakoshh Nov 6, 2024
e70b67b
Merge branch 'master' into 6444-fix-slice-dynamic-index
aakoshh Nov 6, 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
42 changes: 30 additions & 12 deletions compiler/noirc_evaluator/src/ssa/opt/array_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,11 @@
assert_eq!(reachable_blocks.len(), 1, "Expected there to be 1 block remaining in Acir function for array_set optimization");
}

let mut context =
Context::new(&self.dfg, matches!(self.runtime(), RuntimeType::Brillig(_)));
let mut context = Context::new(
&self.dfg,
self.parameters(),
matches!(self.runtime(), RuntimeType::Brillig(_)),
);

for block in reachable_blocks.iter() {
context.analyze_last_uses(*block);
Expand All @@ -50,19 +53,25 @@

struct Context<'f> {
dfg: &'f DataFlowGraph,
function_parameters: &'f [ValueId],
is_brillig_runtime: bool,
array_to_last_use: HashMap<ValueId, InstructionId>,
instructions_that_can_be_made_mutable: HashSet<InstructionId>,
// Mapping of an array that comes from a load and whether the address
// it was loaded from is a reference parameter.
// it was loaded from is a reference parameter passed to the block.
arrays_from_load: HashMap<ValueId, bool>,
inner_nested_arrays: HashMap<ValueId, InstructionId>,
}

impl<'f> Context<'f> {
fn new(dfg: &'f DataFlowGraph, is_brillig_runtime: bool) -> Self {
fn new(
dfg: &'f DataFlowGraph,
function_parameters: &'f [ValueId],
is_brillig_runtime: bool,
) -> Self {
Context {
dfg,
function_parameters,
is_brillig_runtime,
array_to_last_use: HashMap::default(),
instructions_that_can_be_made_mutable: HashSet::default(),
Expand Down Expand Up @@ -109,21 +118,30 @@
// If we are in a return block we are not concerned about the array potentially being mutated again.
let is_return_block =
matches!(terminator, TerminatorInstruction::Return { .. });

// We also want to check that the array is not part of the terminator arguments, as this means it is used again.
let mut array_in_terminator = false;
let mut is_array_in_terminator = false;
terminator.for_each_value(|value| {
// The terminator can contain original IDs, while the SSA has replaced the array value IDs; we need to resolve to compare.
if !array_in_terminator && self.dfg.resolve(value) == array {
array_in_terminator = true;
if !is_array_in_terminator && self.dfg.resolve(value) == array {
is_array_in_terminator = true;
}
});
if let Some(is_from_param) = self.arrays_from_load.get(&array) {

// We cannot safely mutate slices that are inputs to the function, as they might be shared with the caller.
// NB checking the block parameters is not enough, as we might have jumped into a parameterless blocks inside the function.

Check warning on line 132 in compiler/noirc_evaluator/src/ssa/opt/array_set.rs

View workflow job for this annotation

GitHub Actions / Code 

Unknown word (parameterless)
let is_function_param = self.function_parameters.contains(&array);

let can_mutate = if let Some(is_from_param) = self.arrays_from_load.get(&array)
{
// If the array was loaded from a reference parameter, we cannot
// safely mark that array mutable as it may be shared by another value.
if !is_from_param && is_return_block {
self.instructions_that_can_be_made_mutable.insert(*instruction_id);
}
} else if !array_in_terminator {
!is_from_param && is_return_block
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is_from_param also should be looking at function parameters, rather than block parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started here by checking the block parameters, but then it failed on this SSA:

brillig(inline) fn dynamic_slice_index_if f6 {
  b0(v0: u32, v1: [Field], v2: Field):
    v3 = allocate
    store v1 at v3
    inc_rc v1
    v4 = truncate v2 to 32 bits, max_bit_size: 254
    v5 = cast v4 as u32
    v7 = lt v5, u32 10
    jmpif v7 then: b2, else: b1
  b2():
    v13 = cast v2 as u32
    v14 = lt v13, v0
    constrain v14 == u1 1 '"Index out of bounds"'
    v15 = array_get v1, index v13
    constrain v15 == Field 4
    v18 = array_set mut v1, index v13, value Field 2
    store v18 at v3
    jmp b3()
  b3():
    v19 = load v3
    v21 = lt u32 4, v0
    constrain v21 == u1 1 '"Index out of bounds"'
    v22 = array_get v19, index u32 4
    constrain v22 == Field 2
    dec_rc v1
    return 
  b1():
    v8 = cast v2 as u32
    v9 = lt v8, v0
    constrain v9 == u1 1 '"Index out of bounds"'
    v11 = array_get v1, index v8
    constrain v11 == Field 0
    jmp b3()
}

The array_set on v18 is in a block without parameters.

} else {
!is_array_in_terminator && !is_function_param
};

if can_mutate {
self.instructions_that_can_be_made_mutable.insert(*instruction_id);
}
}
Expand Down
Loading