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

fix(ssa): Slice mergers with multiple ifs #2597

Merged
merged 5 commits into from
Sep 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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 @@ -20,6 +20,7 @@ fn regression_dynamic_slice_index(x: Field, y: Field) {

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

fn dynamic_slice_index_set_if(mut slice: [Field], x: Field, y: Field) {
Expand Down Expand Up @@ -164,6 +165,35 @@ fn dynamic_slice_merge_else(mut slice: [Field], x: Field) {
assert(slice[slice.len() - 1] == 20);
}

fn dynamic_slice_merge_two_ifs(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);

if x == 20 {
slice = slice.push_back(20);
} else {
slice = slice.push_back(15);
}
// TODO(#2599): Breaks if the push back happens without the else case
// slice = slice.push_back(15);

assert(slice.len() == 7);
assert(slice[slice.len() - 1] == 15);

slice = slice.push_back(20);
assert(slice.len() == 8);
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);
Expand Down
56 changes: 55 additions & 1 deletion crates/nargo_cli/tests/execution_success/slices/src/main.nr
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,18 @@ fn merge_slices_if(x: Field, y: Field) {
let slice = merge_slices_mutate_in_loop(x, y);
assert(slice[6] == 4);
assert(slice.len() == 7);

let slice = merge_slices_mutate_two_ifs(x, y);
assert(slice.len() == 6);
assert(slice[3] == 5);
assert(slice[4] == 15);
assert(slice[5] == 30);

let slice = merge_slices_mutate_between_ifs(x, y);
assert(slice.len() == 6);
assert(slice[3] == 5);
assert(slice[4] == 30);
assert(slice[5] == 15);
}

fn merge_slices_else(x: Field) {
Expand Down Expand Up @@ -156,4 +168,46 @@ fn merge_slices_mutate_in_loop(x: Field, y: Field) -> [Field] {
slice = slice.push_back(x);
}
slice
}
}

fn merge_slices_mutate_two_ifs(x: Field, y: Field) -> [Field] {
let mut slice = [0; 2];
if x != y {
slice = slice.push_back(y);
slice = slice.push_back(x);
} else {
slice = slice.push_back(x);
}
if x == 20 {
slice = slice.push_back(20);
} else {
slice = slice.push_back(15);
}
// TODO(#2599): Breaks if the push back happens without the else case
// slice = slice.push_back(15);
slice = slice.push_back(30);

slice
}

fn merge_slices_mutate_between_ifs(x: Field, y: Field) -> [Field] {
let mut slice = [0; 2];
if x != y {
slice = slice.push_back(y);
slice = slice.push_back(x);
} else {
slice = slice.push_back(x);
}

slice = slice.push_back(30);

if x == 20 {
slice = slice.push_back(20);
} else {
slice = slice.push_back(15);
}
// TODO(#2599): Breaks if the push back happens without the else case
// slice = slice.push_back(15);

slice
}
2 changes: 1 addition & 1 deletion crates/noirc_evaluator/src/ssa/ir/instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ pub(crate) enum Instruction {
/// Creates a new array with the new value at the given index. All other elements are identical
/// to those in the given array. This will not modify the original array.
///
/// An optional length can be provided to enabling handling of dynamic slice indices
/// An optional length can be provided to enable handling of dynamic slice indices.
ArraySet { array: ValueId, index: ValueId, value: ValueId, length: Option<ValueId> },
}

Expand Down
12 changes: 9 additions & 3 deletions crates/noirc_evaluator/src/ssa/ir/instruction/call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,19 @@ use iter_extended::vecmap;
use num_bigint::BigUint;

use crate::ssa::ir::{
basic_block::BasicBlockId,
dfg::DataFlowGraph,
instruction::Intrinsic,
map::Id,
types::Type,
value::{Value, ValueId}, basic_block::BasicBlockId,
value::{Value, ValueId},
};

use super::{Binary, BinaryOp, Endian, Instruction, SimplifyResult};

/// Try to simplify this call instruction. If the instruction can be simplified to a known value,
/// that value is returned. Otherwise None is returned.
///
///
/// The `block` parameter indicates the block any new instructions that are part of a call's
/// simplification will be inserted into. For example, all slice intrinsics require updates
/// to the slice length, which requires inserting a binary instruction. This update instruction
Expand Down Expand Up @@ -232,7 +233,12 @@ pub(super) fn simplify_call(
/// 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),
/// and not a flattened length used internally to represent arrays of tuples.
fn update_slice_length(slice_len: ValueId, dfg: &mut DataFlowGraph, operator: BinaryOp, block: BasicBlockId) -> ValueId {
fn update_slice_length(
slice_len: ValueId,
dfg: &mut DataFlowGraph,
operator: BinaryOp,
block: BasicBlockId,
) -> ValueId {
let one = dfg.make_constant(FieldElement::one(), Type::field());
let instruction = Instruction::Binary(Binary { lhs: slice_len, operator, rhs: one });
let call_stack = dfg.get_value_call_stack(slice_len);
Expand Down
27 changes: 15 additions & 12 deletions crates/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -489,33 +489,36 @@ impl<'f> Context<'f> {
let value = &self.inserter.function.dfg[value_id];
match value {
Value::Array { array, .. } => array.len(),
Value::NumericConstant { constant, .. } => constant.to_u128() as usize,
Value::Instruction { instruction: instruction_id, .. } => {
let instruction = &self.inserter.function.dfg[*instruction_id];
match instruction {
Instruction::ArraySet { length, .. } => {
let length = length.expect("ICE: array set on a slice must have a length");
self.get_slice_length(length)
}
Instruction::ArraySet { array, .. } => self.get_slice_length(*array),
Instruction::Load { address } => {
let context_store = self
.outer_block_stores
.get(address)
.expect("ICE: load in merger should have store from outer block");
let context_store = if let Some(store) = self.store_values.get(address) {
store
} else {
self.outer_block_stores
.get(address)
.expect("ICE: load in merger should have store from outer block")
};

self.get_slice_length(context_store.new_value)
}
Instruction::Call { func, arguments } => {
let func = &self.inserter.function.dfg[*func];
let length = arguments[0];
let slice_contents = arguments[1];
match func {
Value::Intrinsic(intrinsic) => match intrinsic {
Intrinsic::SlicePushBack
| Intrinsic::SlicePushFront
| Intrinsic::SliceInsert => self.get_slice_length(length) + 1,
| Intrinsic::SliceInsert => {
self.get_slice_length(slice_contents) + 1
}
Intrinsic::SlicePopBack
| Intrinsic::SlicePopFront
| Intrinsic::SliceRemove => self.get_slice_length(length) - 1,
| Intrinsic::SliceRemove => {
self.get_slice_length(slice_contents) - 1
}
_ => {
unreachable!("ICE: Intrinsic not supported, got {intrinsic:?}")
}
Expand Down