From 1cf1408bc8413a37060ccc9e084e04af1c30f292 Mon Sep 17 00:00:00 2001
From: guipublic <guipublic@gmail.com>
Date: Mon, 4 Sep 2023 12:45:05 +0000
Subject: [PATCH 1/8] conditional stores
---
.../noirc_evaluator/src/ssa/acir_gen/mod.rs | 83 ++++++++++++-------
1 file changed, 55 insertions(+), 28 deletions(-)
diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs
index 313322d5d4f..2da51922bc7 100644
--- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs
+++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs
@@ -427,6 +427,15 @@ impl Context {
// assert_eq!(result_ids.len(), outputs.len());
for (result, output) in result_ids.iter().zip(outputs) {
+ // when the intrinsic returns an array, we initialize it
+ if let AcirValue::Array(elements) = &output {
+ let block = self.block_id(result);
+ let mut values = Vec::new();
+ for element in elements {
+ values.push(element.clone());
+ }
+ self.initialize_array(block, elements.len(), Some(&values))?;
+ }
self.ssa_values.insert(*result, output);
}
}
@@ -529,6 +538,10 @@ impl Context {
last_array_uses: &HashMap<ValueId, InstructionId>,
) -> Result<(), RuntimeError> {
let index_const = dfg.get_numeric_constant(index);
+ // we use predicate*index instead of index if there are side effects, in order to avoid index-out-of-bound with false predicates.
+ let index_var = self.convert_numeric_value(index, dfg)?;
+ let predicate_index =
+ self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?;
match self.convert_value(array, dfg) {
AcirValue::Var(acir_var, _) => {
@@ -553,7 +566,7 @@ impl Context {
}
};
if index >= array_size {
- // Ignore the error if side effects are disabled.
+ // Report the error if side effects are disabled.
if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var)
{
let call_stack = self.acir_context.get_call_stack();
@@ -563,23 +576,18 @@ impl Context {
call_stack,
});
}
- let result_type =
- dfg.type_of_value(dfg.instruction_results(instruction)[0]);
- let value = self.create_default_value(&result_type)?;
+ } else {
+ let value = match store_value {
+ Some(store_value) => {
+ let store_value = self.convert_value(store_value, dfg);
+ AcirValue::Array(array.update(index, store_value))
+ }
+ None => array[index].clone(),
+ };
+
self.define_result(dfg, instruction, value);
return Ok(());
}
-
- let value = match store_value {
- Some(store_value) => {
- let store_value = self.convert_value(store_value, dfg);
- AcirValue::Array(array.update(index, store_value))
- }
- None => array[index].clone(),
- };
-
- self.define_result(dfg, instruction, value);
- return Ok(());
}
}
AcirValue::DynamicArray(_) => (),
@@ -587,9 +595,31 @@ impl Context {
let resolved_array = dfg.resolve(array);
let map_array = last_array_uses.get(&resolved_array) == Some(&instruction);
if let Some(store) = store_value {
- self.array_set(instruction, array, index, store, dfg, map_array)?;
+ // In case of side effects:
+ // - we replace the index by predicate*index
+ // - we replace the value by predicate*value + (1-predicate)*dummy, where dummy is the value of the array at the requested index
+ let store_var = self.convert_value(store, dfg).into_var()?;
+ let new_value =
+ if !self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) {
+ // array value at the index
+ let dummy = self.array_get(instruction, array, predicate_index, dfg)?;
+
+ // true_pred = predicate*value
+ let true_pred = self
+ .acir_context
+ .mul_var(store_var, self.current_side_effects_enabled_var)?;
+ let one = self.acir_context.add_constant(FieldElement::one());
+ let not_pred =
+ self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?;
+ // false_pred = (1-predicate)*dummy
+ let false_pred = self.acir_context.mul_var(not_pred, dummy)?;
+ self.acir_context.add_var(true_pred, false_pred)?
+ } else {
+ store_var
+ };
+ self.array_set(instruction, array, predicate_index, new_value, dfg, map_array)?;
} else {
- self.array_get(instruction, array, index, dfg)?;
+ self.array_get(instruction, array, predicate_index, dfg)?;
}
Ok(())
@@ -600,9 +630,9 @@ impl Context {
&mut self,
instruction: InstructionId,
array: ValueId,
- index: ValueId,
+ var_index: AcirVar,
dfg: &DataFlowGraph,
- ) -> Result<(), RuntimeError> {
+ ) -> Result<AcirVar, RuntimeError> {
let array = dfg.resolve(array);
let block_id = self.block_id(&array);
if !self.initialized_arrays.contains(&block_id) {
@@ -616,13 +646,12 @@ impl Context {
return Err(RuntimeError::UnInitialized {
name: "array".to_string(),
call_stack: self.acir_context.get_call_stack(),
- })
+ });
}
}
}
- let index_var = self.convert_value(index, dfg).into_var()?;
- let read = self.acir_context.read_from_memory(block_id, &index_var)?;
+ let read = self.acir_context.read_from_memory(block_id, &var_index)?;
let typ = match dfg.type_of_value(array) {
Type::Array(typ, _) => {
if typ.len() != 1 {
@@ -636,7 +665,7 @@ impl Context {
};
let typ = AcirType::from(typ);
self.define_result(dfg, instruction, AcirValue::Var(read, typ));
- Ok(())
+ Ok(read)
}
/// Copy the array and generates a write opcode on the new array
@@ -646,8 +675,8 @@ impl Context {
&mut self,
instruction: InstructionId,
array: ValueId,
- index: ValueId,
- store_value: ValueId,
+ var_index: AcirVar,
+ store_value: AcirVar,
dfg: &DataFlowGraph,
map_array: bool,
) -> Result<(), InternalError> {
@@ -710,9 +739,7 @@ impl Context {
}
// Write the new value into the new array at the specified index
- let index_var = self.convert_value(index, dfg).into_var()?;
- let value_var = self.convert_value(store_value, dfg).into_var()?;
- self.acir_context.write_to_memory(result_block_id, &index_var, &value_var)?;
+ self.acir_context.write_to_memory(result_block_id, &var_index, &store_value)?;
let result_value =
AcirValue::DynamicArray(AcirDynamicArray { block_id: result_block_id, len });
From 9e18681eaaa95bca8c1e7290b8b2112e04a25185 Mon Sep 17 00:00:00 2001
From: guipublic <guipublic@gmail.com>
Date: Wed, 6 Sep 2023 08:09:50 +0000
Subject: [PATCH 2/8] add test case
---
.../regression_mem_op_predicate/Nargo.toml | 7 +++++++
.../regression_mem_op_predicate/Prover.toml | 2 ++
.../regression_mem_op_predicate/src/main.nr | 8 ++++++++
3 files changed, 17 insertions(+)
create mode 100644 crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/Nargo.toml
create mode 100644 crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/Prover.toml
create mode 100644 crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr
diff --git a/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/Nargo.toml b/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/Nargo.toml
new file mode 100644
index 00000000000..0361b28fd1e
--- /dev/null
+++ b/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/Nargo.toml
@@ -0,0 +1,7 @@
+[package]
+name = "regression_mem_op_predicate"
+type = "bin"
+authors = [""]
+compiler_version = "0.1"
+
+[dependencies]
diff --git a/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/Prover.toml b/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/Prover.toml
new file mode 100644
index 00000000000..3621f24082c
--- /dev/null
+++ b/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/Prover.toml
@@ -0,0 +1,2 @@
+x = [4,3,1,5,111]
+idx = 12
\ No newline at end of file
diff --git a/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr b/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr
new file mode 100644
index 00000000000..d7214e6c601
--- /dev/null
+++ b/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr
@@ -0,0 +1,8 @@
+fn regression_mem_op_predicate(mut x: [u32; 5], idx: Field) {
+ // We should not hit out of bounds here as we have a predicate
+ // that should not be hit
+ if idx as u32 < 3 {
+ x[idx] = 10;
+ }
+ assert(x[4] == 111);
+}
\ No newline at end of file
From 1642c2d6082899802481dacec7fbde44481ae10c Mon Sep 17 00:00:00 2001
From: guipublic <guipublic@gmail.com>
Date: Thu, 7 Sep 2023 07:21:53 +0000
Subject: [PATCH 3/8] code review
---
.../execution_success/regression_mem_op_predicate/src/main.nr | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr b/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr
index d7214e6c601..a592e2b62b4 100644
--- a/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr
+++ b/crates/nargo_cli/tests/execution_success/regression_mem_op_predicate/src/main.nr
@@ -1,4 +1,4 @@
-fn regression_mem_op_predicate(mut x: [u32; 5], idx: Field) {
+fn main(mut x: [u32; 5], idx: Field) {
// We should not hit out of bounds here as we have a predicate
// that should not be hit
if idx as u32 < 3 {
From 1517b64d2be49c122423b5ab891f4de1a86ee2c9 Mon Sep 17 00:00:00 2001
From: jfecher <jake@aztecprotocol.com>
Date: Thu, 7 Sep 2023 13:17:02 -0500
Subject: [PATCH 4/8] Update crates/noirc_evaluator/src/ssa/acir_gen/mod.rs
---
crates/noirc_evaluator/src/ssa/acir_gen/mod.rs | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs
index f039cbedbde..dd41e0b7275 100644
--- a/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs
+++ b/crates/noirc_evaluator/src/ssa/acir_gen/mod.rs
@@ -431,10 +431,7 @@ impl Context {
// when the intrinsic returns an array, we initialize it
if let AcirValue::Array(elements) = &output {
let block = self.block_id(result);
- let mut values = Vec::new();
- for element in elements {
- values.push(element.clone());
- }
+ let values = vecmap(elements, |element| element.clone());
self.initialize_array(block, elements.len(), Some(&values))?;
}
self.ssa_values.insert(*result, output);
From 761e9a59923c3552fe40340e6babb1de0e81ce06 Mon Sep 17 00:00:00 2001
From: guipublic <guipublic@gmail.com>
Date: Wed, 13 Sep 2023 15:11:02 +0000
Subject: [PATCH 5/8] handle all the cases: const/non const index-predicate
yes/no-get/set
---
.../noirc_evaluator/src/ssa/acir_gen/mod.rs | 77 +++++++++++--------
1 file changed, 45 insertions(+), 32 deletions(-)
diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
index 6249a1b2a3f..980e5b5f905 100644
--- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
+++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
@@ -545,12 +545,35 @@ impl Context {
}
};
+ // We compute some AcirVars:
+ // - index_var is the index of the array
+ // - predicate_index is 0, or the index if the predicate is true
+ // - 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
let index_const = dfg.get_numeric_constant(index);
- // we use predicate*index instead of index if there are side effects, in order to avoid index-out-of-bound with false predicates.
let index_var = self.convert_numeric_value(index, dfg)?;
let predicate_index =
self.acir_context.mul_var(index_var, self.current_side_effects_enabled_var)?;
+ let new_value = if let Some(store) = store_value {
+ let store_var = Some(self.convert_value(store, dfg).into_var()?);
+ if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) {
+ store_var
+ } else {
+ let dummy = self.array_get(instruction, array, predicate_index, dfg)?;
+ let true_pred = self
+ .acir_context
+ .mul_var(store_var.unwrap(), self.current_side_effects_enabled_var)?;
+ let one = self.acir_context.add_constant(FieldElement::one());
+ let not_pred =
+ self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?;
+ let false_pred = self.acir_context.mul_var(not_pred, dummy)?;
+ Some(self.acir_context.add_var(true_pred, false_pred)?)
+ }
+ } else {
+ None
+ };
+ // Handle constant index: if there is no predicate and we have the array values, we can perform the operation directly on the array
match dfg.type_of_value(array) {
Type::Array(_, _) => {
match self.convert_value(array, dfg) {
@@ -575,12 +598,11 @@ impl Context {
});
}
};
- if index >= array_size {
- // Ignore the error if side effects are disabled.
- if self
- .acir_context
- .is_constant_one(&self.current_side_effects_enabled_var)
- {
+ if self
+ .acir_context
+ .is_constant_one(&self.current_side_effects_enabled_var)
+ {
+ if index >= array_size {
let call_stack = self.acir_context.get_call_stack();
return Err(RuntimeError::IndexOutOfBounds {
index,
@@ -600,6 +622,11 @@ impl Context {
return Ok(());
}
}
+ // If there is a predicate and the index is not out of range, we can perfom directly the read
+ else if index < array_size && store_value.is_none() {
+ self.define_result(dfg, instruction, array[index].clone());
+ return Ok(());
+ }
}
}
AcirValue::DynamicArray(_) => (),
@@ -611,34 +638,20 @@ impl Context {
_ => unreachable!("ICE: expected array or slice type"),
}
+ let new_index = if self.acir_context.is_constant_one(&self.current_side_effects_enabled_var)
+ {
+ index_var
+ } else {
+ predicate_index
+ };
+
let resolved_array = dfg.resolve(array);
let map_array = last_array_uses.get(&resolved_array) == Some(&instruction);
- if let Some(store) = store_value {
- // In case of side effects:
- // - we replace the index by predicate*index
- // - we replace the value by predicate*value + (1-predicate)*dummy, where dummy is the value of the array at the requested index
- let store_var = self.convert_value(store, dfg).into_var()?;
- let new_value =
- if !self.acir_context.is_constant_one(&self.current_side_effects_enabled_var) {
- // array value at the index
- let dummy = self.array_get(instruction, array, predicate_index, dfg)?;
-
- // true_pred = predicate*value
- let true_pred = self
- .acir_context
- .mul_var(store_var, self.current_side_effects_enabled_var)?;
- let one = self.acir_context.add_constant(FieldElement::one());
- let not_pred =
- self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?;
- // false_pred = (1-predicate)*dummy
- let false_pred = self.acir_context.mul_var(not_pred, dummy)?;
- self.acir_context.add_var(true_pred, false_pred)?
- } else {
- store_var
- };
- self.array_set(instruction, predicate_index, new_value, dfg, map_array)?;
+
+ if let Some(new_value) = new_value {
+ self.array_set(instruction, new_index, new_value, dfg, map_array)?;
} else {
- self.array_get(instruction, array, predicate_index, dfg)?;
+ self.array_get(instruction, array, new_index, dfg)?;
}
Ok(())
From 5179e712c861d77cc595ad3bd79f92c1fca1b3aa Mon Sep 17 00:00:00 2001
From: guipublic <guipublic@gmail.com>
Date: Wed, 13 Sep 2023 15:31:14 +0000
Subject: [PATCH 6/8] code review
---
compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
index 8210ee15882..fdb1dd54543 100644
--- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
+++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
@@ -549,7 +549,8 @@ impl Context {
// - index_var is the index of the array
// - predicate_index is 0, or the index if the predicate is true
// - 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
+ // 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.
let index_const = dfg.get_numeric_constant(index);
let index_var = self.convert_numeric_value(index, dfg)?;
let predicate_index =
@@ -567,6 +568,7 @@ impl Context {
let not_pred =
self.acir_context.sub_var(one, self.current_side_effects_enabled_var)?;
let false_pred = self.acir_context.mul_var(not_pred, dummy)?;
+ // predicate*value + (1-predicate)*dummy
Some(self.acir_context.add_var(true_pred, false_pred)?)
}
} else {
@@ -602,6 +604,7 @@ impl Context {
.acir_context
.is_constant_one(&self.current_side_effects_enabled_var)
{
+ // Report the error if side effects are enabled.
if index >= array_size {
let call_stack = self.acir_context.get_call_stack();
return Err(RuntimeError::IndexOutOfBounds {
From 0ee1651918629111d23002f9158d1f2a8b9eeea2 Mon Sep 17 00:00:00 2001
From: Maxim Vezenov <mvezenov@gmail.com>
Date: Wed, 13 Sep 2023 16:57:00 +0100
Subject: [PATCH 7/8] Update compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
---
compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
index fdb1dd54543..9f3a4d7d886 100644
--- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
+++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
@@ -625,7 +625,7 @@ impl Context {
return Ok(());
}
}
- // If there is a predicate and the index is not out of range, we can perfom directly the read
+ // If there is a predicate and the index is not out of range, we can directly perform the read
else if index < array_size && store_value.is_none() {
self.define_result(dfg, instruction, array[index].clone());
return Ok(());
From 5347cd69418238928a46bd067507ffb8d0ec4c88 Mon Sep 17 00:00:00 2001
From: Maxim Vezenov <mvezenov@gmail.com>
Date: Wed, 13 Sep 2023 16:57:08 +0100
Subject: [PATCH 8/8] Update compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
---
compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 1 -
1 file changed, 1 deletion(-)
diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
index 9f3a4d7d886..a2943596a53 100644
--- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
+++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs
@@ -719,7 +719,6 @@ impl Context {
fn array_set(
&mut self,
instruction: InstructionId,
- // array: ValueId,
var_index: AcirVar,
store_value: AcirVar,
dfg: &DataFlowGraph,