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

feat: Enable dynamic indices on slices #2446

Merged
merged 58 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
1f9c258
initial work for get dynamic indexing for slices working
vezenovm Aug 22, 2023
0c79421
Merge branch 'master' into mv/slice-dyn-index
vezenovm Aug 22, 2023
78c1921
Add everything except alias handling
jfecher Aug 22, 2023
49cc0c8
Fix all tests
jfecher Aug 23, 2023
88913c1
clippy
jfecher Aug 23, 2023
079ea08
initial work to get dyn slices working
vezenovm Aug 23, 2023
0619ad7
Merge branch 'jf/new-mem2reg' into mv/slice-dyn-index
vezenovm Aug 23, 2023
4f22d7b
merged in jf/new-mem2reg, have partial dynamic indices support but ca…
vezenovm Aug 23, 2023
fb77dfa
debug comment
vezenovm Aug 23, 2023
8804a3f
Mild cleanup
jfecher Aug 23, 2023
cd29ec8
Undo some unneeded changes
jfecher Aug 23, 2023
f3f34aa
Partially revert cleanup commit which broke some tests
jfecher Aug 24, 2023
0887db5
Add large doc comment
jfecher Aug 24, 2023
a8a3f36
Merge branch 'master' into jf/new-mem2reg
jfecher Aug 24, 2023
33feb07
Add test and remove printlns
jfecher Aug 24, 2023
2574c33
Remove todos
jfecher Aug 24, 2023
96b9469
working dynamic slice indices w/ slice push back, need to still clean…
vezenovm Aug 24, 2023
240eedb
Merge branch 'jf/new-mem2reg' into mv/slice-dyn-index
vezenovm Aug 24, 2023
cd8cf41
adding slice insert to get_slice_length in flattening, need to rework…
vezenovm Aug 25, 2023
876bde8
remove comment from slice push back
vezenovm Aug 25, 2023
d3dbffa
working dynamic slices except for insert and remove
vezenovm Aug 25, 2023
c44321b
remove old get_slice_length method
vezenovm Aug 25, 2023
d7bf169
some cleanup
vezenovm Aug 25, 2023
7c17c62
conflicts w/ master
vezenovm Aug 25, 2023
b60e23c
cargo clippy and fmt
vezenovm Aug 25, 2023
c672088
remove commented out patch from cargo toml
vezenovm Aug 25, 2023
25d21b4
fix how we resolve length for slice intrinsics in flattening
vezenovm Aug 25, 2023
a1adca3
bring back full slice test
vezenovm Aug 25, 2023
3749b90
uncomment slices test
vezenovm Aug 25, 2023
3b7ef52
remove unnecessary assert that was for testing
vezenovm Aug 25, 2023
71b01a2
Merge branch 'master' into mv/slice-dyn-index
vezenovm Aug 28, 2023
5c30a44
fix outer block stores in flattening
vezenovm Aug 28, 2023
7fb1b11
cargo fmt
vezenovm Aug 28, 2023
cdfa36b
move regression_dynamic_slice_index to new project
vezenovm Aug 28, 2023
d0dded9
switch to var_to_expression and remove get_constant method
vezenovm Aug 28, 2023
347e9a6
slcie insert and remove with constant index
vezenovm Aug 28, 2023
681d3e9
add comments for issues
vezenovm Aug 28, 2023
9b95a3a
Empty-Commit
vezenovm Aug 28, 2023
c0094df
Merge branch 'master' into mv/slice-dyn-index
vezenovm Aug 28, 2023
e067884
fix constant folding optimization and cargo fmt
vezenovm Aug 28, 2023
d093f19
refactor handle array operation
vezenovm Aug 28, 2023
6e4f75a
cargo clipy
vezenovm Aug 28, 2023
15c79e0
remove assert in slice_dynamic_index_test
vezenovm Aug 29, 2023
14849e1
master merge
vezenovm Aug 30, 2023
1ad7de0
remove old debug derive
vezenovm Aug 30, 2023
39fee20
add assert constant in slice insert and remove
vezenovm Sep 1, 2023
8afe885
fix brillig slices
vezenovm Sep 1, 2023
1d0c822
master conflcits
vezenovm Sep 1, 2023
e6d7aa8
pass correct inputs for recursive aggregation but we need to most lik…
vezenovm Sep 1, 2023
c38e873
Merge branch 'master' into mv/slice-dyn-index
vezenovm Sep 5, 2023
395e79c
merge conflicts
vezenovm Sep 5, 2023
dc2defb
use default after fxHashMap
vezenovm Sep 5, 2023
6446e86
Merge branch 'master' into mv/slice-dyn-index
vezenovm Sep 5, 2023
cbf6f36
fix call_black_box for recursive agg
vezenovm Sep 5, 2023
a14f9f3
cleanup printer and cargo fmt
vezenovm Sep 5, 2023
2c695e3
remove assert_constant case from brillig_block
vezenovm Sep 6, 2023
1e346be
remove arguments.len() check in simplify_call ArrayLen
vezenovm Sep 6, 2023
576b15c
add test for using push back on dyn slice outside of if statement
vezenovm Sep 6, 2023
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
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,4 @@ tower = "0.4"
url = "2.2.0"
wasm-bindgen = { version = "=0.2.86", features = ["serde-serialize"] }
wasm-bindgen-test = "0.3.33"
base64 = "0.21.2"
base64 = "0.21.2"
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "slice_dynamic_index"
type = "bin"
authors = [""]
compiler_version = "0.10.3"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
x = "5"
Original file line number Diff line number Diff line change
@@ -0,0 +1,222 @@
fn main(x : Field) {
// The parameters to this function must come directly from witness values (inputs to main).
regression_dynamic_slice_index(x - 1, x - 4);
jfecher marked this conversation as resolved.
Show resolved Hide resolved
}

fn regression_dynamic_slice_index(x: Field, y: Field) {
let mut slice = [];
for i in 0..5 {
slice = slice.push_back(i);
}
assert(slice.len() == 5);

dynamic_slice_index_set_if(slice, x, y);
dynamic_slice_index_set_else(slice, x, y);
dynamic_slice_index_set_nested_if_else_else(slice, x, y);
dynamic_slice_index_set_nested_if_else_if(slice, x, y + 1);
dynamic_slice_index_if(slice, x);
dynamic_array_index_if([0, 1, 2, 3, 4], x);
dynamic_slice_index_else(slice, x);

dynamic_slice_merge_if(slice, x);
dynamic_slice_merge_else(slice, x);
}

fn dynamic_slice_index_set_if(mut slice: [Field], x: Field, y: Field) {
assert(slice[x] == 4);
assert(slice[y] == 1);
slice[y] = 0;
assert(slice[x] == 4);
assert(slice[1] == 0);
if x as u32 < 10 {
assert(slice[x] == 4);
slice[x] = slice[x] - 2;
slice[x - 1] = slice[x];
} else {
slice[x] = 0;
}
assert(slice[3] == 2);
assert(slice[4] == 2);
}

fn dynamic_slice_index_set_else(mut slice: [Field], x: Field, y: Field) {
assert(slice[x] == 4);
assert(slice[y] == 1);
slice[y] = 0;
assert(slice[x] == 4);
assert(slice[1] == 0);
if x as u32 > 10 {
assert(slice[x] == 4);
slice[x] = slice[x] - 2;
slice[x - 1] = slice[x];
} else {
slice[x] = 0;
}
assert(slice[4] == 0);
}

// This tests the case of missing a store instruction in the else branch
// of merging slices
fn dynamic_slice_index_if(mut slice: [Field], x: Field) {
if x as u32 < 10 {
assert(slice[x] == 4);
slice[x] = slice[x] - 2;
} else {
assert(slice[x] == 0);
}
assert(slice[4] == 2);
}

fn dynamic_array_index_if(mut array: [Field; 5], x: Field) {
if x as u32 < 10 {
assert(array[x] == 4);
array[x] = array[x] - 2;
} else {
assert(array[x] == 0);
}
assert(array[4] == 2);
}

// This tests the case of missing a store instruction in the then branch
// of merging slices
fn dynamic_slice_index_else(mut slice: [Field], x: Field) {
if x as u32 > 10 {
assert(slice[x] == 0);
} else {
assert(slice[x] == 4);
slice[x] = slice[x] - 2;
}
assert(slice[4] == 2);
}


fn dynamic_slice_merge_if(mut slice: [Field], x: Field) {
if x as u32 < 10 {
assert(slice[x] == 4);
slice[x] = slice[x] - 2;

slice = slice.push_back(10);
// Having an array set here checks whether we appropriately
// handle a slice length that is not yet resolving to a constant
// during flattening
slice[x] = 10;
assert(slice[slice.len() - 1] == 10);
assert(slice.len() == 6);

slice[x] = 20;
slice[x] = slice[x] + 10;

slice = slice.push_front(11);
assert(slice[0] == 11);
assert(slice.len() == 7);
assert(slice[5] == 30);

slice = slice.push_front(12);
assert(slice[0] == 12);
assert(slice.len() == 8);
assert(slice[6] == 30);

let (popped_slice, last_elem) = slice.pop_back();
assert(last_elem == 10);
assert(popped_slice.len() == 7);

let (first_elem, rest_of_slice) = popped_slice.pop_front();
assert(first_elem == 12);
assert(rest_of_slice.len() == 6);

// TODO(#2462): SliceInsert and SliceRemove with a dynamic index are not yet implemented in ACIR gen
slice = rest_of_slice.insert(2, 20);
assert(slice[2] == 20);
assert(slice[6] == 30);
assert(slice.len() == 7);

// TODO(#2462): SliceInsert and SliceRemove with a dynamic index are not yet implemented in ACIR gen
let (removed_slice, removed_elem) = slice.remove(3);
// The deconstructed tuple assigns to the slice but is not seen outside of the if statement
// without a direct assignment
slice = removed_slice;

assert(removed_elem == 1);
assert(slice.len() == 6);
} else {
assert(slice[x] == 0);
slice = slice.push_back(20);
}

assert(slice.len() == 6);
assert(slice[slice.len() - 1] == 30);
}

fn dynamic_slice_merge_else(mut slice: [Field], x: Field) {
if x as u32 > 10 {
assert(slice[x] == 0);
slice[x] = 2;
} else {
assert(slice[x] == 4);
slice[x] = slice[x] - 2;
slice = slice.push_back(10);
}
assert(slice.len() == 6);
assert(slice[slice.len() - 1] == 10);

slice = slice.push_back(20);
assert(slice.len() == 7);
assert(slice[slice.len() - 1] == 20);
}

fn dynamic_slice_index_set_nested_if_else_else(mut slice: [Field], x: Field, y: Field) {
assert(slice[x] == 4);
assert(slice[y] == 1);
slice[y] = 0;
assert(slice[x] == 4);
assert(slice[1] == 0);
if x as u32 < 10 {
slice[x] = slice[x] - 2;
if y != 1 {
slice[x] = slice[x] + 20;
} else {
if x == 5 {
// We should not hit this case
assert(slice[x] == 22);
} else {
slice[x] = 10;
slice = slice.push_back(15);
assert(slice.len() == 6);
}
assert(slice[4] == 10);
}
} else {
slice[x] = 0;
}
assert(slice[4] == 10);
assert(slice.len() == 6);
assert(slice[slice.len() - 1] == 15);

slice = slice.push_back(20);
assert(slice.len() == 7);
assert(slice[slice.len() - 1] == 20);
}

fn dynamic_slice_index_set_nested_if_else_if(mut slice: [Field], x: Field, y: Field) {
assert(slice[x] == 4);
assert(slice[y] == 2);
slice[y] = 0;
assert(slice[x] == 4);
assert(slice[2] == 0);
if x as u32 < 10 {
slice[x] = slice[x] - 2;
// TODO: this panics as we have a load for the slice in flattening
if y == 1 {
slice[x] = slice[x] + 20;
} else {
if x == 4 {
slice[x] = 5;
}
assert(slice[4] == 5);
}
} else {
slice[x] = 0;
}
assert(slice[4] == 5);
}

3 changes: 2 additions & 1 deletion crates/nargo_cli/tests/execution_success/slices/src/main.nr
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use dep::std::slice;
use dep::std;

fn main(x : Field, y : pub Field) {
let mut slice = [0; 2];
assert(slice[0] == 0);
Expand Down Expand Up @@ -155,4 +156,4 @@ fn merge_slices_mutate_in_loop(x: Field, y: Field) -> [Field] {
slice = slice.push_back(x);
}
slice
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@
destination_variable,
);
}
Instruction::ArraySet { array, index, value } => {
Instruction::ArraySet { array, index, value, .. } => {
let source_variable = self.convert_ssa_value(*array, dfg);
let index_register = self.convert_ssa_register_value(*index, dfg);
let value_variable = self.convert_ssa_value(*value, dfg);
Expand Down Expand Up @@ -858,7 +858,7 @@

/// Slices have a tuple structure (slice length, slice contents) to enable logic
/// that uses dynamic slice lengths (such as with merging slices in the flattening pass).
/// This method codegens an update to the slice length.

Check warning on line 861 in crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (codegens)
///
/// The binary operation performed on the slice length is always an addition or subtraction of `1`.
/// This is because the slice length holds the user length (length as displayed by a `.len()` call),
Expand Down
3 changes: 2 additions & 1 deletion crates/noirc_evaluator/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ impl RuntimeError {
}
_ => {
let message = self.to_string();
let location = self.call_stack().back().expect("Expected RuntimeError to have a location");
let location =
self.call_stack().back().expect("Expected RuntimeError to have a location");

Diagnostic::simple_error(message, String::new(), location.span)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
FieldElement,
};
use acvm::{BlackBoxFunctionSolver, BlackBoxResolutionError};
use fxhash::FxHashMap as HashMap;

Check warning on line 23 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (fxhash)
use iter_extended::{try_vecmap, vecmap};
use std::{borrow::Cow, hash::Hash};

Expand Down Expand Up @@ -240,7 +240,7 @@
}

/// Converts an [`AcirVar`] to an [`Expression`]
fn var_to_expression(&self, var: AcirVar) -> Result<Expression, InternalError> {
pub(crate) fn var_to_expression(&self, var: AcirVar) -> Result<Expression, InternalError> {
let var_data = match self.vars.get(&var) {
Some(var_data) => var_data,
None => {
Expand Down Expand Up @@ -285,7 +285,7 @@
let inverted_var = self.add_data(AcirVarData::Const(constant.inverse()));

// Check that the inverted var is valid.
// This check prevents invalid divisons by zero.

Check warning on line 288 in crates/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (divisons)
let should_be_one = self.mul_var(inverted_var, var)?;
self.maybe_eq_predicate(should_be_one, predicate)?;

Expand Down
Loading
Loading