fix(ssa): Change array_set to not mutate slices coming from function inputs #6463
Conversation
…function as it might be shared
| self.instructions_that_can_be_made_mutable.insert(*instruction_id); | ||
| } | ||
| } else if !array_in_terminator { | ||
| !is_from_param && is_return_block |
There was a problem hiding this comment.
I wonder if this is_from_param also should be looking at function parameters, rather than block parameters?
There was a problem hiding this comment.
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.
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
|
This only affects brillig code, right? If so, we should probably ignore the mutability on array sets completely there - that was only ever intended for Acir. Brillig should use the ref counts on each array to determine mutability. |
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Only brillig tests failed with this.
No I'm mixing it up with something. Ah yes, I read this on the
|
|
So you're saying that if Brillig ignored the |
The original intention was for Acir to only use the mutable field (and for that pass to only be for acir code), and for brillig to only use ref counts for mutation. |
Description
Problem*
Resolves #6444
Summary*
#6355 changed the logic in the
array_setSSA pass so that arrays which were loaded from references that are inputs to the block are not set with mutation, as the reference might be shared with something outside the block.In this instance what we had was a slice passed to a function like
v1: [Field], as opposed to the above which passed a referencev1: &mut [Field; 3], but the effect was similar in that the code ended up mutating the slice backingv1, over which a refcount was even increased, instead of making a copy.The PR changes the decision so that if we are setting an item on an array that is part of the function inputs (not the block inputs), then it will not use mutation.
Additional Context
See #6444 (comment) for the SSA samples.
I have tested this with #6429 but opened the PR to
master.Documentation*
Check one:
PR Checklist*
cargo fmton default settings.