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: set index and value to 0 for array_get with predicate #4971

Merged
merged 14 commits into from
May 7, 2024
71 changes: 64 additions & 7 deletions compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -957,13 +957,39 @@ impl<'a> Context<'a> {
return Ok(());
}

let (new_index, new_value) =
self.convert_array_operation_inputs(array, dfg, index, store_value)?;
// Get an offset such that the type of the array at the offset is the same as the type at the 'index'
// If we find one, we will use it when computing the index under the enable_side_effect predicate
// If not, array_get(..) will use a fallback costing one multiplication in the worst case.
// cf. https://github.com/noir-lang/noir/pull/4971
let mut offset = None;
let array_id = dfg.resolve(array);
let array_typ = dfg.type_of_value(array_id);
// For simplicity we compute the offset only for simple arrays
if dfg.instruction_results(instruction).len() == 1
&& can_omit_element_sizes_array(&array_typ)
{
let result_type = dfg.type_of_value(dfg.instruction_results(instruction)[0]);
if let Type::Array(item_type, _) | Type::Slice(item_type) = array_typ {
for (i, typ) in item_type.iter().enumerate() {
if result_type == *typ {
offset = Some(i);
break;
}
}
}
}
guipublic marked this conversation as resolved.
Show resolved Hide resolved
let (new_index, new_value) = self.convert_array_operation_inputs(
array,
dfg,
index,
store_value,
offset.unwrap_or_default(),
)?;

if let Some(new_value) = new_value {
self.array_set(instruction, new_index, new_value, dfg, mutable_array_set)?;
} else {
self.array_get(instruction, array, new_index, dfg)?;
self.array_get(instruction, array, new_index, dfg, offset.is_none())?;
}

Ok(())
Expand Down Expand Up @@ -1054,6 +1080,7 @@ impl<'a> Context<'a> {
/// in case we have a nested array. The index for SSA array operations only represents the flattened index of the current array.
/// Thus internal array element type sizes need to be computed to accurately transform the index.
/// - predicate_index is 0, or the index if the predicate is true
/// if an offset is specified, predicate_index is index*predicate + (1-predicate)*offset
guipublic marked this conversation as resolved.
Show resolved Hide resolved
/// - new_value is the optional value when the operation is an array_set
/// When there is a predicate, it is predicate*value + (1-predicate)*dummy, where dummy is the value of the array at the requested index.
/// It is a dummy value because in the case of a false predicate, the value stored at the requested index will be itself.
Expand All @@ -1063,14 +1090,21 @@ impl<'a> Context<'a> {
dfg: &DataFlowGraph,
index: ValueId,
store_value: Option<ValueId>,
offset: usize,
) -> Result<(AcirVar, Option<AcirValue>), RuntimeError> {
let (array_id, array_typ, block_id) = self.check_array_is_initialized(array, dfg)?;

let index_var = self.convert_numeric_value(index, dfg)?;
let index_var = self.get_flattened_index(&array_typ, array_id, index_var, dfg)?;

let predicate_index =
self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?;
let predicate_index = if offset != 0 {
let offset = self.acir_context.add_constant(offset);
let sub = self.acir_context.sub_var(index_var, offset)?;
vezenovm marked this conversation as resolved.
Show resolved Hide resolved
let pred = self.acir_context.mul_var(sub, self.current_side_effects_enabled_var)?;
self.acir_context.add_var(pred, offset)?
} else {
self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?
};
guipublic marked this conversation as resolved.
Show resolved Hide resolved

let new_value = if let Some(store) = store_value {
let store_value = self.convert_value(store, dfg);
Expand Down Expand Up @@ -1171,12 +1205,14 @@ impl<'a> Context<'a> {
}

/// Generates a read opcode for the array
/// index_side_effect false means that we ensured index will have a type matching the value in the array
guipublic marked this conversation as resolved.
Show resolved Hide resolved
fn array_get(
&mut self,
instruction: InstructionId,
array: ValueId,
mut var_index: AcirVar,
dfg: &DataFlowGraph,
mut index_side_effect: bool,
) -> Result<AcirValue, RuntimeError> {
let (array_id, _, block_id) = self.check_array_is_initialized(array, dfg)?;
let results = dfg.instruction_results(instruction);
Expand All @@ -1195,7 +1231,7 @@ impl<'a> Context<'a> {
self.data_bus.call_data_map[&array_id] as i128,
));
let new_index = self.acir_context.add_var(offset, bus_index)?;
return self.array_get(instruction, call_data, new_index, dfg);
return self.array_get(instruction, call_data, new_index, dfg, index_side_effect);
}
}

Expand All @@ -1204,7 +1240,28 @@ impl<'a> Context<'a> {
!res_typ.contains_slice_element(),
"ICE: Nested slice result found during ACIR generation"
);
let value = self.array_get_value(&res_typ, block_id, &mut var_index)?;
let mut value = self.array_get_value(&res_typ, block_id, &mut var_index)?;

if let AcirValue::Var(value_var, typ) = &value {
let array_id = dfg.resolve(array_id);
let array_typ = dfg.type_of_value(array_id);
if let (Type::Numeric(numeric_type), AcirType::NumericType(num)) =
(array_typ.first(), typ)
{
if numeric_type.bit_size() <= num.bit_size() {
// first element is compatible
index_side_effect = false;
}
}
// Fallback to multiplication if the index side_effects have not already been handled
if index_side_effect {
// Set the value to 0 if current_side_effects is 0, to ensure it fits in any value type
value = AcirValue::Var(
self.acir_context.mul_var(*value_var, self.current_side_effects_enabled_var)?,
typ.clone(),
);
}
}

self.define_result(dfg, instruction, value.clone());

Expand Down
8 changes: 8 additions & 0 deletions compiler/noirc_evaluator/src/ssa/ir/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,14 @@ impl Type {
other => panic!("element_types: Expected array or slice, found {other}"),
}
}

pub(crate) fn first(&self) -> Type {
match self {
Type::Numeric(_) | Type::Function => self.clone(),
Type::Reference(typ) => typ.first(),
Type::Slice(element_types) | Type::Array(element_types, _) => element_types[0].first(),
}
}
}

/// Composite Types are essentially flattened struct or tuple types.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "regression_struct_array_conditional"
version = "0.1.0"
type = "bin"
authors = [""]

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
y = 1
z = 1

[[x]]
value = "0x23de33be058ce5504e1ade738db8bdacfe268fa9dbde777092bf1d38519bdf59"
counter = "10"
dummy = "0"

[[x]]
value = "3"
counter = "2"
dummy = "0"

[[x]]
value = "2"
counter = "0"
dummy = "0"

Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
struct foo {
value: Field,
counter: u8,
dummy: u8,
}
struct bar {
dummy: [u8;3],
value: Field,
counter: u8,
}
struct bar_field {
dummy: [Field;3],
value: Field,
counter: u8,
}
fn main(x: [foo; 3], y: u32, z: u32) -> pub u8 {
let a = [y, z, x[y].counter as u32];
let mut b = [bar { value: 0, counter: 0, dummy: [0; 3] }; 3];
let mut c = [bar_field { value: 0, counter: 0, dummy: [0; 3] }; 3];
for i in 0..3 {
b[i].value = x[i].value;
b[i].counter = x[i].counter;
b[i].dummy[0] = x[i].dummy;
c[i].value = x[i].value;
c[i].counter = x[i].counter;
c[i].dummy[0] = x[i].dummy as Field;
}
if z == 0 {
// offset
assert(y as u8 < x[y].counter);
assert(y <= a[y]);
// first element is compatible
assert(y as u8 < b[y].counter);
// fallback
assert(y as u8 < c[y].counter);
}
x[0].counter
}
Loading