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): Tracking slices stores correctly for parent blocks and handling in ACIR #4221

Closed
wants to merge 8 commits into from
Closed
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
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ impl RuntimeError {
_ => {
let message = self.to_string();
let location =
self.call_stack().back().expect("Expected RuntimeError to have a location");
self.call_stack().back().unwrap_or_else(|| panic!("Expected RuntimeError to have a location. Error message: {message}"));

Diagnostic::simple_error(message, String::new(), location.span)
}
Expand Down
92 changes: 86 additions & 6 deletions compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -661,11 +661,11 @@ impl Context {
instruction: InstructionId,
dfg: &DataFlowGraph,
index: ValueId,
array: ValueId,
array_id: ValueId,
store_value: Option<ValueId>,
) -> Result<bool, RuntimeError> {
let index_const = dfg.get_numeric_constant(index);
let value_type = dfg.type_of_value(array);
let value_type = dfg.type_of_value(array_id);
// Compiler sanity checks
assert!(
!value_type.is_nested_slice(),
Expand All @@ -675,7 +675,7 @@ impl Context {
unreachable!("ICE: expected array or slice type");
};

match self.convert_value(array, dfg) {
match self.convert_value(array_id, dfg) {
AcirValue::Var(acir_var, _) => {
return Err(RuntimeError::InternalError(InternalError::Unexpected {
expected: "an array value".to_string(),
Expand All @@ -699,7 +699,7 @@ impl Context {
};
if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) {
// Report the error if side effects are enabled.
if index >= array_size {
if index >= array_size && !matches!(value_type, Type::Slice(_)) {
let call_stack = self.acir_context.get_call_stack();
return Err(RuntimeError::IndexOutOfBounds {
index,
Expand All @@ -712,7 +712,17 @@ impl Context {
let store_value = self.convert_value(store_value, dfg);
AcirValue::Array(array.update(index, store_value))
}
None => array[index].clone(),
None => {
// If the index is out of range for a slice we should directly create dummy data.
// This situation should only happen during flattening of slices when the same slice after an if statement
// would have different lengths depending upon the predicate.
// Otherwise, we can potentially hit an index out of bounds error.
if index >= array_size {
self.construct_dummy_slice_value(instruction, dfg)
} else {
array[index].clone()
}
}
};

self.define_result(dfg, instruction, value);
Expand All @@ -724,14 +734,84 @@ impl Context {
self.define_result(dfg, instruction, array[index].clone());
return Ok(true);
}
// If there is a non constant predicate and the index is out of range for a slice, we should directly create dummy data.
// This situation should only happen during flattening of slices when the same slice after an if statement
// would have different lengths depending upon the predicate.
// Otherwise, we can potentially hit an index out of bounds error.
else if index >= array_size
&& value_type.contains_slice_element()
&& store_value.is_none()
{
if index >= array_size {
let value = self.construct_dummy_slice_value(instruction, dfg);

self.define_result(dfg, instruction, value);
return Ok(true);
} else {
return Ok(false);
}
}
}
}
AcirValue::DynamicArray(_) => (),
AcirValue::DynamicArray(AcirDynamicArray { len, .. }) => {
if let Some(index_const) = index_const {
let index = match index_const.try_to_u64() {
Some(index_const) => index_const as usize,
None => {
let call_stack = self.acir_context.get_call_stack();
return Err(RuntimeError::TypeConversion {
from: "array index".to_string(),
into: "u64".to_string(),
call_stack,
});
}
};

if index >= len && matches!(value_type, Type::Slice(_)) {
let value = self.construct_dummy_slice_value(instruction, dfg);

self.define_result(dfg, instruction, value);
return Ok(true);
}
}

return Ok(false);
}
};

Ok(false)
}

fn construct_dummy_slice_value(
&mut self,
instruction: InstructionId,
dfg: &DataFlowGraph,
) -> AcirValue {
let results = dfg.instruction_results(instruction);
let res_typ = dfg.type_of_value(results[0]);
self.construct_dummy_slice_value_inner(&res_typ)
}

fn construct_dummy_slice_value_inner(&mut self, ssa_type: &Type) -> AcirValue {
match ssa_type.clone() {
Type::Numeric(numeric_type) => {
let zero = self.acir_context.add_constant(FieldElement::zero());
let typ = AcirType::NumericType(numeric_type);
AcirValue::Var(zero, typ)
}
Type::Array(element_types, len) => {
let mut values = Vector::new();
for _ in 0..len {
for typ in element_types.as_ref() {
values.push_back(self.construct_dummy_slice_value_inner(typ));
}
}
AcirValue::Array(values)
}
_ => unreachable!("ICE - expected an array or numeric type"),
}
}

/// We need to properly setup the inputs for array operations in ACIR.
/// From the original SSA values we compute the following AcirVars:
/// - new_index is the index of the array. ACIR memory operations work with a flat memory, so we fully flattened the specified index
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ impl<'f> Context<'f> {
/// Returns the last block to be inlined. This is either the return block of the function or,
/// if self.conditions is not empty, the end block of the most recent condition.
fn handle_terminator(&mut self, block: BasicBlockId) -> BasicBlockId {
if let TerminatorInstruction::JmpIf { .. } =
if let TerminatorInstruction::JmpIf { .. } | TerminatorInstruction::Jmp { .. } =
self.inserter.function.dfg[block].unwrap_terminator()
{
// Find stores in the outer block and insert into the `outer_block_stores` map.
Expand Down
7 changes: 7 additions & 0 deletions test_programs/execution_success/regression_4202/Nargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[package]
name = "regression_4202"
type = "bin"
authors = [""]
compiler_version = ">=0.23.0"

[dependencies]
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
input = [1, 2, 3, 4]
14 changes: 14 additions & 0 deletions test_programs/execution_success/regression_4202/src/main.nr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
fn main(input: [u32; 4]) {
let mut slice1: [u32] = [1, 2, 3, 4];
if slice1[0] == 3 {
slice1[1] = 4;
}

if slice1[1] == 5 {
slice1[3] = 6;
}

for i in 0..4 {
assert(slice1[i] == input[i]);
}
}
Loading