fix: do not eliminate possible out of bounds errors #5610
Conversation
There was a problem hiding this comment.
Is there another way to do this? That is, can we not remove these instructions when we don't know if they are out of bounds?
None that I know of.
I am a bit wary of this PR though. Since it seems like a potential performance degradation in larger programs and on slices. E.g. programs which make use of more dynamic indexing or more slices will be hit harder by this.
Changes to circuit sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Agreed. I'll take a look at the regressions to see if there are cases where they can still be removed in case the index is not known. |
|
I wonder if it'd help constraint counts at all if we treated all |
|
Good idea! I was looking at the fn main(x: Field) {
let mut slice = &[0, 1, 2, 3, 4];
let _ = slice[x - 1];
}The SSA before this PR is this: acir(inline) fn main f0 {
b0(v0: Field):
v24 = sub v0, Field 1
v25 = cast v24 as u32
v26 = lt v25, u32 5
constrain v26 == u1 1 '"Index out of bounds"'
return
}The new SSA is this: acir(inline) fn main f0 {
b0(v0: Field):
v24 = sub v0, Field 1
v25 = cast v24 as u32
v26 = lt v25, u32 5
constrain v26 == u1 1 '"Index out of bounds"'
v27 = array_get [Field 0, Field 1, Field 2, Field 3, Field 4], index v25
return
}So two things:
What's interesting is that this program has a different SSA: fn main(x: Field) {
let mut slice = &[0, 1, 2, 3, 4];
slice[x - 1] = 1;
}The SSA before this PR is this one (empty!): acir(inline) fn main f0 {
b0(v0: Field):
return
}In this PR it's this: acir(inline) fn main f0 {
b0(v0: Field):
v23 = sub v0, Field 1
v24 = cast v23 as u32
v25 = array_set mut [Field 0, Field 1, Field 2, Field 3, Field 4], index v24, value Field 1
return
}I was looking at our code and there's |
|
However... that constrain only seems to be inserted for slices, not for arrays... not sure why. |
…:noir-lang/noir into ab/do-not-remove-array-get-out-of-bounds
Discussed on a call, but posting here visibility as well. We currently only code gen a length constrain on slices due to their lengths being dynamic. I see a couple options:
I lean towards option 2 as although it may cause some larger programs, it should only cause larger programs where we have unused array gets/sets. I think it is ok to say that a developer should remove these themselves. We can add an unused variable warning array gets/sets that are later unused, and then in the long-term perhaps we can have a "release" mode that removes these automatically. |
I think unused gets & sets are more widespread than you think since in the 3 slices tests that increased in size in the CI this leads to a 3-5% increase in constraint counts which seems significant. I'm leaning towards 1 currently so that we treat arrays & slices the same, then continue to treat get/set as pure. I'd be interested in the constraint count difference but hopefully it shouldn't be much since we already perform this check just within the get/set. |
|
Just a note: originally the PR kept |
|
@asterite it is automatically updated with each commit you make as part of the "Report gates diff" CI action |
|
To summarize all the scenarios... here are four scenarios and whether they correctly give you "index out of bounds". Array getKnown index out of boundsfn main() {
let array = [1, 2, 3];
let _ = array[10];
}Expected: index out of bounds Unknown indexfn main(x: Field) {
let array = [1, 2, 3];
let _ = array[x]; // x is 10 in Prover.toml
}Expected: index out of bounds Array setKnown index out of boundsfn main() {
let mut array = [1, 2, 3];
array[10] = 42;
}Expected: index out of bounds Unknown indexfn main(x: Field) {
let mut array = [1, 2, 3];
array[x] = 42; // x is 10 in Prover.toml
}Expected: index out of bounds Slice getKnown index out of boundsfn main() {
let slice = &[1, 2, 3];
let _ = slice[10];
}Expected: index out of bounds Unknown indexfn main(x: Field) {
let slice = &[1, 2, 3];
let _ = slice[x]; // x is 10 in Prover.toml
}Expected: index out of bounds Slice setKnown index out of boundsfn main() {
let mut slice = &[1, 2, 3];
slice[10] = 42;
}Expected: index out of bounds Unknown indexfn main(x: Field) {
let mut slice = &[1, 2, 3];
slice[x] = 42; // x is 10 in Prover.toml
}Expected: index out of bounds ConclusionI'm not sure how to proceed here. It seems a lot of checks are missing but I don't know if they should go into SSA or acir-gen. |
I think the way to proceed is to always insert |
|
Closed in favor of #5676 |
|
Closing because we chose #5691 instead. |
Description
Problem
Resolves #5296
Summary
Is there another way to do this? That is, can we not remove these instructions when we don't know if they are out of bounds?
Additional Context
None.
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.