From 3080801a8870a215f368b53c50b2557095113a32 Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 30 Oct 2024 11:09:08 +0000 Subject: [PATCH 01/11] right shift is not a regular division --- .../src/ssa/ir/instruction/binary.rs | 10 +----- .../src/ssa/opt/remove_bit_shifts.rs | 33 +++++++++++++------ .../bit_shifts_comptime/src/main.nr | 4 +++ .../bit_shifts_runtime/src/main.nr | 4 ++- 4 files changed, 31 insertions(+), 20 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index 03262be0a06..487370488b9 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -281,15 +281,7 @@ impl Binary { let zero = dfg.make_constant(FieldElement::zero(), operand_type); return SimplifyResult::SimplifiedTo(zero); } - - // `two_pow_rhs` is limited to be at most `2 ^ {operand_bitsize - 1}` so it fits in `operand_type`. - let two_pow_rhs = FieldElement::from(2u128).pow(&rhs_const); - let two_pow_rhs = dfg.make_constant(two_pow_rhs, operand_type); - return SimplifyResult::SimplifiedToInstruction(Instruction::binary( - BinaryOp::Div, - self.lhs, - two_pow_rhs, - )); + return SimplifyResult::None; } } }; diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index 6f3f2fa14b7..cdbb1043232 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -145,6 +145,8 @@ impl Context<'_> { } /// Insert ssa instructions which computes lhs >> rhs by doing lhs/2^rhs + /// For negative signed integers, we do the division on the 1-complement representation of lhs, + /// before converting back the result to the 2-complement representation. pub(crate) fn insert_shift_right( &mut self, lhs: ValueId, @@ -153,16 +155,27 @@ impl Context<'_> { ) -> ValueId { let lhs_typ = self.function.dfg.type_of_value(lhs); let base = self.field_constant(FieldElement::from(2_u128)); - // we can safely cast to unsigned because overflow_checks prevent bit-shift with a negative value - let rhs_unsigned = self.insert_cast(rhs, Type::unsigned(bit_size)); - let pow = self.pow(base, rhs_unsigned); - // We need at least one more bit for the case where rhs == bit_size - let div_type = Type::unsigned(bit_size + 1); - let casted_lhs = self.insert_cast(lhs, div_type.clone()); - let casted_pow = self.insert_cast(pow, div_type); - let div_result = self.insert_binary(casted_lhs, BinaryOp::Div, casted_pow); - // We have to cast back to the original type - self.insert_cast(div_result, lhs_typ) + let pow = self.pow(base, rhs); + if lhs_typ.is_unsigned() { + // unsigned right bit shift is just a normal division + self.insert_binary(lhs, BinaryOp::Div, pow) + } else { + // Get the sign of the operand; positive signed operand will just do a division as well + let zero = self.numeric_constant(FieldElement::zero(), Type::signed(bit_size)); + let lhs_sign = self.insert_binary(lhs, BinaryOp::Lt, zero); + let lhs_sign_as_field = self.insert_cast(lhs_sign, Type::field()); + let lhs_as_field = self.insert_cast(lhs, Type::field()); + // For negative numbers, convert to 1-complement using wrapping addition of a + 1 + let one_complement = self.insert_binary(lhs_sign_as_field, BinaryOp::Add, lhs_as_field); + let one_complement = self.insert_truncate(one_complement, bit_size, bit_size + 1); + let one_complement = self.insert_cast(one_complement, Type::signed(bit_size)); + // Performs the division on the 1-complement (or the operand if positive) + let shifted_complement = self.insert_binary(one_complement, BinaryOp::Div, pow); + // Convert back to 2-complement representation if operand is negative + let lhs_sign_as_int = self.insert_cast(lhs_sign, lhs_typ); + let shifted = self.insert_binary(shifted_complement, BinaryOp::Sub, lhs_sign_as_int); + self.insert_truncate(shifted, bit_size, bit_size + 1) + } } /// Computes lhs^rhs via square&multiply, using the bits decomposition of rhs diff --git a/test_programs/execution_success/bit_shifts_comptime/src/main.nr b/test_programs/execution_success/bit_shifts_comptime/src/main.nr index 6d9736b6abb..a11dae1c716 100644 --- a/test_programs/execution_success/bit_shifts_comptime/src/main.nr +++ b/test_programs/execution_success/bit_shifts_comptime/src/main.nr @@ -15,6 +15,10 @@ fn main(x: u64) { assert(x << 63 == 0); assert_eq((1 as u64) << 32, 0x0100000000); + + //regression for 6201 + let a: i16 = -769; + assert_eq(a >> 3, -97); } fn regression_2250() { diff --git a/test_programs/execution_success/bit_shifts_runtime/src/main.nr b/test_programs/execution_success/bit_shifts_runtime/src/main.nr index 059bbe84dac..6374e2d4a63 100644 --- a/test_programs/execution_success/bit_shifts_runtime/src/main.nr +++ b/test_programs/execution_success/bit_shifts_runtime/src/main.nr @@ -1,4 +1,4 @@ -fn main(x: u64, y: u8) { +fn main(x: u64, y: u8, a: i16) { // runtime shifts on compile-time known values assert(64 << y == 128); assert(64 >> y == 32); @@ -17,4 +17,6 @@ fn main(x: u64, y: u8) { assert(a << y == -2); assert(x >> (x as u8) == 0); + + assert_eq(a >> 3, -97); } From fc1ea5b85dfba213193305c7a8e95a645e11a2b3 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 31 Oct 2024 09:26:08 +0000 Subject: [PATCH 02/11] code review --- .../execution_success/bit_shifts_runtime/Prover.toml | 3 ++- .../execution_success/bit_shifts_runtime/src/main.nr | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/test_programs/execution_success/bit_shifts_runtime/Prover.toml b/test_programs/execution_success/bit_shifts_runtime/Prover.toml index 98d8630792e..548c07ac07b 100644 --- a/test_programs/execution_success/bit_shifts_runtime/Prover.toml +++ b/test_programs/execution_success/bit_shifts_runtime/Prover.toml @@ -1,2 +1,3 @@ x = 64 -y = 1 \ No newline at end of file +y = 1 +z = "-769" \ No newline at end of file diff --git a/test_programs/execution_success/bit_shifts_runtime/src/main.nr b/test_programs/execution_success/bit_shifts_runtime/src/main.nr index 6374e2d4a63..370bb699048 100644 --- a/test_programs/execution_success/bit_shifts_runtime/src/main.nr +++ b/test_programs/execution_success/bit_shifts_runtime/src/main.nr @@ -1,4 +1,4 @@ -fn main(x: u64, y: u8, a: i16) { +fn main(x: u64, y: u8, z: i16) { // runtime shifts on compile-time known values assert(64 << y == 128); assert(64 >> y == 32); @@ -18,5 +18,5 @@ fn main(x: u64, y: u8, a: i16) { assert(x >> (x as u8) == 0); - assert_eq(a >> 3, -97); + assert_eq(z >> 3, -97); } From d3cdebd2c9f79dbdefb093437335dc9d810b7df9 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 31 Oct 2024 11:11:56 +0000 Subject: [PATCH 03/11] ensure ACIR is solvable --- acvm-repo/acvm/src/compiler/mod.rs | 2 + .../compiler/optimizers/merge_expressions.rs | 27 +-- acvm-repo/acvm/src/compiler/simulator.rs | 221 ++++++++++++++++++ tooling/nargo/src/ops/transform.rs | 20 +- tooling/nargo_cli/src/cli/compile_cmd.rs | 2 +- tooling/nargo_cli/src/cli/dap_cmd.rs | 4 +- tooling/nargo_cli/src/cli/debug_cmd.rs | 4 +- 7 files changed, 257 insertions(+), 23 deletions(-) create mode 100644 acvm-repo/acvm/src/compiler/simulator.rs diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 92e03cc90c2..8829f77e50b 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -7,10 +7,12 @@ use acir::{ // The various passes that we can use over ACIR mod optimizers; +mod simulator; mod transformers; pub use optimizers::optimize; use optimizers::optimize_internal; +pub use simulator::CircuitSimulator; use transformers::transform_internal; pub use transformers::{transform, MIN_EXPRESSION_WIDTH}; diff --git a/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs b/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs index ddf86f60f77..7c0be0fc3fe 100644 --- a/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs +++ b/acvm-repo/acvm/src/compiler/optimizers/merge_expressions.rs @@ -6,6 +6,8 @@ use acir::{ AcirField, }; +use crate::compiler::CircuitSimulator; + pub(crate) struct MergeExpressionsOptimizer { resolved_blocks: HashMap>, } @@ -76,7 +78,7 @@ impl MergeExpressionsOptimizer { modified_gates.insert(b, Opcode::AssertZero(expr)); to_keep = false; // Update the 'used_witness' map to account for the merge. - for w2 in Self::expr_wit(&expr_define) { + for w2 in CircuitSimulator::expr_wit(&expr_define) { if !circuit_inputs.contains(&w2) { let mut v = used_witness[&w2].clone(); v.insert(b); @@ -104,22 +106,15 @@ impl MergeExpressionsOptimizer { (new_circuit, new_acir_opcode_positions) } - fn expr_wit(expr: &Expression) -> BTreeSet { - let mut result = BTreeSet::new(); - result.extend(expr.mul_terms.iter().flat_map(|i| vec![i.1, i.2])); - result.extend(expr.linear_combinations.iter().map(|i| i.1)); - result - } - fn brillig_input_wit(&self, input: &BrilligInputs) -> BTreeSet { let mut result = BTreeSet::new(); match input { BrilligInputs::Single(expr) => { - result.extend(Self::expr_wit(expr)); + result.extend(CircuitSimulator::expr_wit(expr)); } BrilligInputs::Array(exprs) => { for expr in exprs { - result.extend(Self::expr_wit(expr)); + result.extend(CircuitSimulator::expr_wit(expr)); } } BrilligInputs::MemoryArray(block_id) => { @@ -134,16 +129,16 @@ impl MergeExpressionsOptimizer { fn witness_inputs(&self, opcode: &Opcode) -> BTreeSet { let mut witnesses = BTreeSet::new(); match opcode { - Opcode::AssertZero(expr) => Self::expr_wit(expr), + Opcode::AssertZero(expr) => CircuitSimulator::expr_wit(expr), Opcode::BlackBoxFuncCall(bb_func) => bb_func.get_input_witnesses(), - Opcode::Directive(Directive::ToLeRadix { a, .. }) => Self::expr_wit(a), + Opcode::Directive(Directive::ToLeRadix { a, .. }) => CircuitSimulator::expr_wit(a), Opcode::MemoryOp { block_id: _, op, predicate } => { //index et value, et predicate let mut witnesses = BTreeSet::new(); - witnesses.extend(Self::expr_wit(&op.index)); - witnesses.extend(Self::expr_wit(&op.value)); + witnesses.extend(CircuitSimulator::expr_wit(&op.index)); + witnesses.extend(CircuitSimulator::expr_wit(&op.value)); if let Some(p) = predicate { - witnesses.extend(Self::expr_wit(p)); + witnesses.extend(CircuitSimulator::expr_wit(p)); } witnesses } @@ -162,7 +157,7 @@ impl MergeExpressionsOptimizer { witnesses.insert(*i); } if let Some(p) = predicate { - witnesses.extend(Self::expr_wit(p)); + witnesses.extend(CircuitSimulator::expr_wit(p)); } witnesses } diff --git a/acvm-repo/acvm/src/compiler/simulator.rs b/acvm-repo/acvm/src/compiler/simulator.rs new file mode 100644 index 00000000000..f9c0b76d548 --- /dev/null +++ b/acvm-repo/acvm/src/compiler/simulator.rs @@ -0,0 +1,221 @@ +use acir::{ + circuit::{ + brillig::{BrilligInputs, BrilligOutputs}, + directives::Directive, + opcodes::{BlockId, FunctionInput}, + Circuit, Opcode, + }, + native_types::{Expression, Witness}, + AcirField, +}; +use std::collections::{BTreeSet, HashMap, HashSet}; + +#[derive(PartialEq)] +enum BlockStatus { + Initialized, + Used, +} + +/// Simulate a symbolic solve for a circuit +#[derive(Default)] +pub struct CircuitSimulator { + /// Track the witnesses that can be solved + solvable_witness: HashSet, + + /// Tells whether a Memory Block is: + /// - Not initialized if not in the map + /// - Initialized if its status is Initialized in the Map + /// - Used, indicating that the block cannot be written anymore. + resolved_blocks: HashMap, +} + +impl CircuitSimulator { + // Simulate a symbolic solve for a circuit by keeping track of the witnesses that can be solved. + // Returns false if the circuit cannot be solved + pub fn check_circuit(&mut self, circuit: &Circuit) -> bool { + let circuit_inputs = circuit.circuit_arguments(); + self.solvable_witness.extend(circuit_inputs.iter()); + for op in &circuit.opcodes { + if !self.try_solve(op) { + return false; + } + } + true + } + + /// Check if the Opcode can be solved, and if yes, add the solved witness to set of solvable witness + fn try_solve(&mut self, opcode: &Opcode) -> bool { + let mut unresolved = HashSet::new(); + match opcode { + Opcode::AssertZero(expr) => { + for (_, w1, w2) in &expr.mul_terms { + if !self.solvable_witness.contains(w1) { + if !self.solvable_witness.contains(w2) { + return false; + } + unresolved.insert(*w1); + } + if !self.solvable_witness.contains(w2) && w1 != w2 { + unresolved.insert(*w2); + } + } + for (_, w) in &expr.linear_combinations { + if !self.solvable_witness.contains(w) { + unresolved.insert(*w); + } + } + if unresolved.len() == 1 { + self.mark_solvable(*unresolved.iter().next().unwrap()); + return true; + } + unresolved.is_empty() + } + Opcode::BlackBoxFuncCall(black_box_func_call) => { + let inputs = black_box_func_call.get_inputs_vec(); + for input in inputs { + if !self.can_solve_function_input(&input) { + return false; + } + } + let outputs = black_box_func_call.get_outputs_vec(); + for output in outputs { + self.mark_solvable(output); + } + true + } + Opcode::Directive(directive) => match directive { + Directive::ToLeRadix { a, b, .. } => { + if !self.can_solve_expression(a) { + return false; + } + for w in b { + self.mark_solvable(*w); + } + true + } + }, + Opcode::MemoryOp { block_id, op, predicate } => { + if !self.can_solve_expression(&op.index) { + return false; + } + if let Some(predicate) = predicate { + if !self.can_solve_expression(predicate) { + return false; + } + } + if op.operation.is_zero() { + let w = op.value.to_witness().unwrap(); + self.mark_solvable(w); + true + } else { + if let Some(BlockStatus::Used) = self.resolved_blocks.get(block_id) { + // Writing after having used the block should not be allowed + return false; + } + self.try_solve(&Opcode::AssertZero(op.value.clone())) + } + } + Opcode::MemoryInit { block_id, init, .. } => { + for w in init { + if !self.solvable_witness.contains(w) { + return false; + } + } + self.resolved_blocks.insert(*block_id, BlockStatus::Initialized); + true + } + Opcode::BrilligCall { id: _, inputs, outputs, predicate } => { + for input in inputs { + if !self.can_solve_brillig_input(input) { + return false; + } + } + if let Some(predicate) = predicate { + if !self.can_solve_expression(predicate) { + return false; + } + } + for output in outputs { + match output { + BrilligOutputs::Simple(w) => self.mark_solvable(*w), + BrilligOutputs::Array(arr) => { + for w in arr { + self.mark_solvable(*w); + } + } + } + } + true + } + Opcode::Call { id: _, inputs, outputs, predicate } => { + for w in inputs { + if !self.solvable_witness.contains(w) { + return false; + } + } + if let Some(predicate) = predicate { + if !self.can_solve_expression(predicate) { + return false; + } + } + for w in outputs { + self.mark_solvable(*w); + } + true + } + } + } + + /// Adds the witness to set of solvable witness + pub(crate) fn mark_solvable(&mut self, witness: Witness) { + self.solvable_witness.insert(witness); + } + + pub fn can_solve_function_input(&self, input: &FunctionInput) -> bool { + if !input.is_constant() { + return self.solvable_witness.contains(&input.to_witness()); + } + true + } + fn can_solve_expression(&self, expr: &Expression) -> bool { + for w in Self::expr_wit(expr) { + if !self.solvable_witness.contains(&w) { + return false; + } + } + true + } + fn can_solve_brillig_input(&mut self, input: &BrilligInputs) -> bool { + match input { + BrilligInputs::Single(expr) => self.can_solve_expression(expr), + BrilligInputs::Array(exprs) => { + for expr in exprs { + if !self.can_solve_expression(expr) { + return false; + } + } + true + } + + BrilligInputs::MemoryArray(block_id) => match self.resolved_blocks.entry(*block_id) { + std::collections::hash_map::Entry::Vacant(_) => false, + std::collections::hash_map::Entry::Occupied(entry) + if *entry.get() == BlockStatus::Used => + { + true + } + std::collections::hash_map::Entry::Occupied(mut entry) => { + entry.insert(BlockStatus::Used); + true + } + }, + } + } + + pub(crate) fn expr_wit(expr: &Expression) -> BTreeSet { + let mut result = BTreeSet::new(); + result.extend(expr.mul_terms.iter().flat_map(|i| vec![i.1, i.2])); + result.extend(expr.linear_combinations.iter().map(|i| i.1)); + result + } +} diff --git a/tooling/nargo/src/ops/transform.rs b/tooling/nargo/src/ops/transform.rs index 9255ac3e0ec..21d599a0b83 100644 --- a/tooling/nargo/src/ops/transform.rs +++ b/tooling/nargo/src/ops/transform.rs @@ -1,21 +1,33 @@ use acvm::{ acir::circuit::{ExpressionWidth, Program}, + compiler::CircuitSimulator, FieldElement, }; use iter_extended::vecmap; -use noirc_driver::{CompiledContract, CompiledProgram}; -use noirc_errors::debug_info::DebugInfo; +use noirc_driver::{CompilationResult, CompiledContract, CompiledProgram}; +use noirc_errors::{debug_info::DebugInfo, CustomDiagnostic, FileDiagnostic}; pub fn transform_program( mut compiled_program: CompiledProgram, expression_width: ExpressionWidth, -) -> CompiledProgram { +) -> CompilationResult { compiled_program.program = transform_program_internal( compiled_program.program, &mut compiled_program.debug, expression_width, ); - compiled_program + // Check if the program is solvable + for circuit in &mut compiled_program.program.functions { + let mut simulator = CircuitSimulator::default(); + if !simulator.check_circuit(circuit) { + let diag = FileDiagnostic { + file_id: fm::FileId::dummy(), + diagnostic: CustomDiagnostic::from_message("ACVM simulation failed"), + }; + return Err(vec![diag]); + } + } + Ok((compiled_program, Vec::new())) } pub fn transform_contract( diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index 18fb407d413..f6a75c3f0e3 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -191,7 +191,7 @@ fn compile_programs( let target_width = get_target_width(package.expression_width, compile_options.expression_width); - let program = nargo::ops::transform_program(program, target_width); + let program = nargo::ops::transform_program(program, target_width)?.0; save_program_to_file(&program.into(), &package.name, workspace.target_directory_path()); Ok(((), warnings)) diff --git a/tooling/nargo_cli/src/cli/dap_cmd.rs b/tooling/nargo_cli/src/cli/dap_cmd.rs index a84e961cfe7..dcc28d22b5d 100644 --- a/tooling/nargo_cli/src/cli/dap_cmd.rs +++ b/tooling/nargo_cli/src/cli/dap_cmd.rs @@ -119,7 +119,9 @@ fn load_and_compile_project( ) .map_err(|_| LoadError::Generic("Failed to compile project".into()))?; - let compiled_program = nargo::ops::transform_program(compiled_program, expression_width); + let compiled_program = nargo::ops::transform_program(compiled_program, expression_width) + .map_err(|err| LoadError::Generic(err[0].diagnostic.to_string()))? + .0; let (inputs_map, _) = read_inputs_from_file(&package.root_dir, prover_name, Format::Toml, &compiled_program.abi) diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index e837f297475..1cad37c0999 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -83,7 +83,9 @@ pub(crate) fn run(args: DebugCommand, config: NargoConfig) -> Result<(), CliErro let target_width = get_target_width(package.expression_width, args.compile_options.expression_width); - let compiled_program = nargo::ops::transform_program(compiled_program, target_width); + let compiled_program = nargo::ops::transform_program(compiled_program, target_width) + .map_err(|err| CliError::Generic(err[0].diagnostic.to_string()))? + .0; run_async(package, compiled_program, &args.prover_name, &args.witness_name, target_dir) } From b4314a9e4068d9ce943f89880580a32ce1829620 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 31 Oct 2024 13:11:40 +0000 Subject: [PATCH 04/11] revert bit shift changes from another branch --- .../src/ssa/ir/instruction/binary.rs | 10 +++++- .../src/ssa/opt/remove_bit_shifts.rs | 33 ++++++------------- .../bit_shifts_comptime/src/main.nr | 4 --- .../bit_shifts_runtime/Prover.toml | 1 - .../bit_shifts_runtime/src/main.nr | 2 -- 5 files changed, 19 insertions(+), 31 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs index 487370488b9..03262be0a06 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/instruction/binary.rs @@ -281,7 +281,15 @@ impl Binary { let zero = dfg.make_constant(FieldElement::zero(), operand_type); return SimplifyResult::SimplifiedTo(zero); } - return SimplifyResult::None; + + // `two_pow_rhs` is limited to be at most `2 ^ {operand_bitsize - 1}` so it fits in `operand_type`. + let two_pow_rhs = FieldElement::from(2u128).pow(&rhs_const); + let two_pow_rhs = dfg.make_constant(two_pow_rhs, operand_type); + return SimplifyResult::SimplifiedToInstruction(Instruction::binary( + BinaryOp::Div, + self.lhs, + two_pow_rhs, + )); } } }; diff --git a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs index cdbb1043232..6f3f2fa14b7 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/remove_bit_shifts.rs @@ -145,8 +145,6 @@ impl Context<'_> { } /// Insert ssa instructions which computes lhs >> rhs by doing lhs/2^rhs - /// For negative signed integers, we do the division on the 1-complement representation of lhs, - /// before converting back the result to the 2-complement representation. pub(crate) fn insert_shift_right( &mut self, lhs: ValueId, @@ -155,27 +153,16 @@ impl Context<'_> { ) -> ValueId { let lhs_typ = self.function.dfg.type_of_value(lhs); let base = self.field_constant(FieldElement::from(2_u128)); - let pow = self.pow(base, rhs); - if lhs_typ.is_unsigned() { - // unsigned right bit shift is just a normal division - self.insert_binary(lhs, BinaryOp::Div, pow) - } else { - // Get the sign of the operand; positive signed operand will just do a division as well - let zero = self.numeric_constant(FieldElement::zero(), Type::signed(bit_size)); - let lhs_sign = self.insert_binary(lhs, BinaryOp::Lt, zero); - let lhs_sign_as_field = self.insert_cast(lhs_sign, Type::field()); - let lhs_as_field = self.insert_cast(lhs, Type::field()); - // For negative numbers, convert to 1-complement using wrapping addition of a + 1 - let one_complement = self.insert_binary(lhs_sign_as_field, BinaryOp::Add, lhs_as_field); - let one_complement = self.insert_truncate(one_complement, bit_size, bit_size + 1); - let one_complement = self.insert_cast(one_complement, Type::signed(bit_size)); - // Performs the division on the 1-complement (or the operand if positive) - let shifted_complement = self.insert_binary(one_complement, BinaryOp::Div, pow); - // Convert back to 2-complement representation if operand is negative - let lhs_sign_as_int = self.insert_cast(lhs_sign, lhs_typ); - let shifted = self.insert_binary(shifted_complement, BinaryOp::Sub, lhs_sign_as_int); - self.insert_truncate(shifted, bit_size, bit_size + 1) - } + // we can safely cast to unsigned because overflow_checks prevent bit-shift with a negative value + let rhs_unsigned = self.insert_cast(rhs, Type::unsigned(bit_size)); + let pow = self.pow(base, rhs_unsigned); + // We need at least one more bit for the case where rhs == bit_size + let div_type = Type::unsigned(bit_size + 1); + let casted_lhs = self.insert_cast(lhs, div_type.clone()); + let casted_pow = self.insert_cast(pow, div_type); + let div_result = self.insert_binary(casted_lhs, BinaryOp::Div, casted_pow); + // We have to cast back to the original type + self.insert_cast(div_result, lhs_typ) } /// Computes lhs^rhs via square&multiply, using the bits decomposition of rhs diff --git a/test_programs/execution_success/bit_shifts_comptime/src/main.nr b/test_programs/execution_success/bit_shifts_comptime/src/main.nr index a11dae1c716..6d9736b6abb 100644 --- a/test_programs/execution_success/bit_shifts_comptime/src/main.nr +++ b/test_programs/execution_success/bit_shifts_comptime/src/main.nr @@ -15,10 +15,6 @@ fn main(x: u64) { assert(x << 63 == 0); assert_eq((1 as u64) << 32, 0x0100000000); - - //regression for 6201 - let a: i16 = -769; - assert_eq(a >> 3, -97); } fn regression_2250() { diff --git a/test_programs/execution_success/bit_shifts_runtime/Prover.toml b/test_programs/execution_success/bit_shifts_runtime/Prover.toml index 548c07ac07b..67bf6a6a234 100644 --- a/test_programs/execution_success/bit_shifts_runtime/Prover.toml +++ b/test_programs/execution_success/bit_shifts_runtime/Prover.toml @@ -1,3 +1,2 @@ x = 64 y = 1 -z = "-769" \ No newline at end of file diff --git a/test_programs/execution_success/bit_shifts_runtime/src/main.nr b/test_programs/execution_success/bit_shifts_runtime/src/main.nr index 370bb699048..f71b74421b7 100644 --- a/test_programs/execution_success/bit_shifts_runtime/src/main.nr +++ b/test_programs/execution_success/bit_shifts_runtime/src/main.nr @@ -17,6 +17,4 @@ fn main(x: u64, y: u8, z: i16) { assert(a << y == -2); assert(x >> (x as u8) == 0); - - assert_eq(z >> 3, -97); } From 7f6ae6d8fe71582b1d9534d94220489a78ffab28 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 31 Oct 2024 13:16:32 +0000 Subject: [PATCH 05/11] missed one --- test_programs/execution_success/bit_shifts_runtime/src/main.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test_programs/execution_success/bit_shifts_runtime/src/main.nr b/test_programs/execution_success/bit_shifts_runtime/src/main.nr index f71b74421b7..059bbe84dac 100644 --- a/test_programs/execution_success/bit_shifts_runtime/src/main.nr +++ b/test_programs/execution_success/bit_shifts_runtime/src/main.nr @@ -1,4 +1,4 @@ -fn main(x: u64, y: u8, z: i16) { +fn main(x: u64, y: u8) { // runtime shifts on compile-time known values assert(64 << y == 128); assert(64 >> y == 32); From 148800333310111fd705b3cd30a37ec001b24152 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 31 Oct 2024 18:04:45 +0000 Subject: [PATCH 06/11] Move check circuit outside of transform --- compiler/wasm/src/compile.rs | 1 + compiler/wasm/src/compile_new.rs | 1 + tooling/nargo/src/ops/check.rs | 18 ++++++++++++++++++ tooling/nargo/src/ops/mod.rs | 2 ++ tooling/nargo/src/ops/transform.rs | 21 +++++---------------- tooling/nargo_cli/src/cli/compile_cmd.rs | 3 ++- tooling/nargo_cli/src/cli/dap_cmd.rs | 4 +--- tooling/nargo_cli/src/cli/debug_cmd.rs | 4 +--- 8 files changed, 31 insertions(+), 23 deletions(-) create mode 100644 tooling/nargo/src/ops/check.rs diff --git a/compiler/wasm/src/compile.rs b/compiler/wasm/src/compile.rs index 05f42bc91a1..4e2765a78e7 100644 --- a/compiler/wasm/src/compile.rs +++ b/compiler/wasm/src/compile.rs @@ -180,6 +180,7 @@ pub fn compile_program( .0; let optimized_program = nargo::ops::transform_program(compiled_program, expression_width); + nargo::ops::check_program(&optimized_program)?; let warnings = optimized_program.warnings.clone(); Ok(JsCompileProgramResult::new(optimized_program.into(), warnings)) diff --git a/compiler/wasm/src/compile_new.rs b/compiler/wasm/src/compile_new.rs index ef2af1dd654..f01bb718fd6 100644 --- a/compiler/wasm/src/compile_new.rs +++ b/compiler/wasm/src/compile_new.rs @@ -118,6 +118,7 @@ impl CompilerContext { .0; let optimized_program = nargo::ops::transform_program(compiled_program, expression_width); + nargo::ops::check_program(&optimized_program)?; let warnings = optimized_program.warnings.clone(); Ok(JsCompileProgramResult::new(optimized_program.into(), warnings)) diff --git a/tooling/nargo/src/ops/check.rs b/tooling/nargo/src/ops/check.rs new file mode 100644 index 00000000000..b976970f693 --- /dev/null +++ b/tooling/nargo/src/ops/check.rs @@ -0,0 +1,18 @@ +use acvm::compiler::CircuitSimulator; +use noirc_driver::{CompiledProgram, ErrorsAndWarnings}; +use noirc_errors::{CustomDiagnostic, FileDiagnostic}; + +pub fn check_program(compiled_program: &CompiledProgram) -> Result<(), ErrorsAndWarnings> { + // Check if the program is solvable + for circuit in &compiled_program.program.functions { + let mut simulator = CircuitSimulator::default(); + if !simulator.check_circuit(circuit) { + let diag = FileDiagnostic { + file_id: fm::FileId::dummy(), + diagnostic: CustomDiagnostic::from_message("ACVM simulation failed"), + }; + return Err(vec![diag]); + } + } + Ok(()) +} diff --git a/tooling/nargo/src/ops/mod.rs b/tooling/nargo/src/ops/mod.rs index 16680dab980..f70577a14f1 100644 --- a/tooling/nargo/src/ops/mod.rs +++ b/tooling/nargo/src/ops/mod.rs @@ -1,3 +1,4 @@ +pub use self::check::check_program; pub use self::compile::{ collect_errors, compile_contract, compile_program, compile_program_with_debug_instrumenter, compile_workspace, report_errors, @@ -9,6 +10,7 @@ pub use self::transform::{transform_contract, transform_program}; pub use self::test::{run_test, TestStatus}; +mod check; mod compile; mod execute; mod foreign_calls; diff --git a/tooling/nargo/src/ops/transform.rs b/tooling/nargo/src/ops/transform.rs index 21d599a0b83..da73a649258 100644 --- a/tooling/nargo/src/ops/transform.rs +++ b/tooling/nargo/src/ops/transform.rs @@ -1,33 +1,22 @@ use acvm::{ acir::circuit::{ExpressionWidth, Program}, - compiler::CircuitSimulator, FieldElement, }; use iter_extended::vecmap; -use noirc_driver::{CompilationResult, CompiledContract, CompiledProgram}; -use noirc_errors::{debug_info::DebugInfo, CustomDiagnostic, FileDiagnostic}; +use noirc_driver::{CompiledContract, CompiledProgram}; +use noirc_errors::debug_info::DebugInfo; pub fn transform_program( mut compiled_program: CompiledProgram, expression_width: ExpressionWidth, -) -> CompilationResult { +) -> CompiledProgram { compiled_program.program = transform_program_internal( compiled_program.program, &mut compiled_program.debug, expression_width, ); - // Check if the program is solvable - for circuit in &mut compiled_program.program.functions { - let mut simulator = CircuitSimulator::default(); - if !simulator.check_circuit(circuit) { - let diag = FileDiagnostic { - file_id: fm::FileId::dummy(), - diagnostic: CustomDiagnostic::from_message("ACVM simulation failed"), - }; - return Err(vec![diag]); - } - } - Ok((compiled_program, Vec::new())) + + compiled_program } pub fn transform_contract( diff --git a/tooling/nargo_cli/src/cli/compile_cmd.rs b/tooling/nargo_cli/src/cli/compile_cmd.rs index f6a75c3f0e3..304988ed516 100644 --- a/tooling/nargo_cli/src/cli/compile_cmd.rs +++ b/tooling/nargo_cli/src/cli/compile_cmd.rs @@ -191,7 +191,8 @@ fn compile_programs( let target_width = get_target_width(package.expression_width, compile_options.expression_width); - let program = nargo::ops::transform_program(program, target_width)?.0; + let program = nargo::ops::transform_program(program, target_width); + nargo::ops::check_program(&program)?; save_program_to_file(&program.into(), &package.name, workspace.target_directory_path()); Ok(((), warnings)) diff --git a/tooling/nargo_cli/src/cli/dap_cmd.rs b/tooling/nargo_cli/src/cli/dap_cmd.rs index dcc28d22b5d..a84e961cfe7 100644 --- a/tooling/nargo_cli/src/cli/dap_cmd.rs +++ b/tooling/nargo_cli/src/cli/dap_cmd.rs @@ -119,9 +119,7 @@ fn load_and_compile_project( ) .map_err(|_| LoadError::Generic("Failed to compile project".into()))?; - let compiled_program = nargo::ops::transform_program(compiled_program, expression_width) - .map_err(|err| LoadError::Generic(err[0].diagnostic.to_string()))? - .0; + let compiled_program = nargo::ops::transform_program(compiled_program, expression_width); let (inputs_map, _) = read_inputs_from_file(&package.root_dir, prover_name, Format::Toml, &compiled_program.abi) diff --git a/tooling/nargo_cli/src/cli/debug_cmd.rs b/tooling/nargo_cli/src/cli/debug_cmd.rs index 1cad37c0999..e837f297475 100644 --- a/tooling/nargo_cli/src/cli/debug_cmd.rs +++ b/tooling/nargo_cli/src/cli/debug_cmd.rs @@ -83,9 +83,7 @@ pub(crate) fn run(args: DebugCommand, config: NargoConfig) -> Result<(), CliErro let target_width = get_target_width(package.expression_width, args.compile_options.expression_width); - let compiled_program = nargo::ops::transform_program(compiled_program, target_width) - .map_err(|err| CliError::Generic(err[0].diagnostic.to_string()))? - .0; + let compiled_program = nargo::ops::transform_program(compiled_program, target_width); run_async(package, compiled_program, &args.prover_name, &args.witness_name, target_dir) } From 7007f989a069cdf941247ac363bae0229c1f2073 Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 31 Oct 2024 18:06:10 +0000 Subject: [PATCH 07/11] revert transform --- tooling/nargo/src/ops/transform.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tooling/nargo/src/ops/transform.rs b/tooling/nargo/src/ops/transform.rs index da73a649258..9255ac3e0ec 100644 --- a/tooling/nargo/src/ops/transform.rs +++ b/tooling/nargo/src/ops/transform.rs @@ -15,7 +15,6 @@ pub fn transform_program( &mut compiled_program.debug, expression_width, ); - compiled_program } From 6a820b60e4ee577a00323857bbc29fb9c421f416 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 1 Nov 2024 12:09:24 +0000 Subject: [PATCH 08/11] chore: fix compilation error --- compiler/wasm/src/compile.rs | 8 +++++++- compiler/wasm/src/compile_new.rs | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/compiler/wasm/src/compile.rs b/compiler/wasm/src/compile.rs index 4e2765a78e7..e823f90add5 100644 --- a/compiler/wasm/src/compile.rs +++ b/compiler/wasm/src/compile.rs @@ -180,7 +180,13 @@ pub fn compile_program( .0; let optimized_program = nargo::ops::transform_program(compiled_program, expression_width); - nargo::ops::check_program(&optimized_program)?; + nargo::ops::check_program(&optimized_program).map_err(|errs| { + CompileError::with_file_diagnostics( + "Compiled program is not solvable", + errs, + &context.file_manager, + ) + })?; let warnings = optimized_program.warnings.clone(); Ok(JsCompileProgramResult::new(optimized_program.into(), warnings)) diff --git a/compiler/wasm/src/compile_new.rs b/compiler/wasm/src/compile_new.rs index f01bb718fd6..ac2f79147b3 100644 --- a/compiler/wasm/src/compile_new.rs +++ b/compiler/wasm/src/compile_new.rs @@ -118,7 +118,13 @@ impl CompilerContext { .0; let optimized_program = nargo::ops::transform_program(compiled_program, expression_width); - nargo::ops::check_program(&optimized_program)?; + nargo::ops::check_program(&optimized_program).map_err(|errs| { + CompileError::with_file_diagnostics( + "Compiled program is not solvable", + errs, + &self.context.file_manager, + ) + })?; let warnings = optimized_program.warnings.clone(); Ok(JsCompileProgramResult::new(optimized_program.into(), warnings)) From 51a8598d2bfa832b0ba5a6be585b771fcea9b02a Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 1 Nov 2024 12:25:19 +0000 Subject: [PATCH 09/11] chore: improve error message --- tooling/nargo/src/ops/check.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tooling/nargo/src/ops/check.rs b/tooling/nargo/src/ops/check.rs index b976970f693..14d629ab0f6 100644 --- a/tooling/nargo/src/ops/check.rs +++ b/tooling/nargo/src/ops/check.rs @@ -4,12 +4,15 @@ use noirc_errors::{CustomDiagnostic, FileDiagnostic}; pub fn check_program(compiled_program: &CompiledProgram) -> Result<(), ErrorsAndWarnings> { // Check if the program is solvable - for circuit in &compiled_program.program.functions { + for (i, circuit) in compiled_program.program.functions.iter().enumerate() { let mut simulator = CircuitSimulator::default(); if !simulator.check_circuit(circuit) { let diag = FileDiagnostic { file_id: fm::FileId::dummy(), - diagnostic: CustomDiagnostic::from_message("ACVM simulation failed"), + diagnostic: CustomDiagnostic::from_message(&format!( + "Circuit \"{}\" is not solvable", + compiled_program.names[i] + )), }; return Err(vec![diag]); } From 13a2999b43db2c9f94d77d88d4056489d37151b3 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 1 Nov 2024 12:34:21 +0000 Subject: [PATCH 10/11] chore: add some sanity check tests --- acvm-repo/acvm/src/compiler/simulator.rs | 83 ++++++++++++++++++++++++ 1 file changed, 83 insertions(+) diff --git a/acvm-repo/acvm/src/compiler/simulator.rs b/acvm-repo/acvm/src/compiler/simulator.rs index f9c0b76d548..3a38769a160 100644 --- a/acvm-repo/acvm/src/compiler/simulator.rs +++ b/acvm-repo/acvm/src/compiler/simulator.rs @@ -219,3 +219,86 @@ impl CircuitSimulator { result } } + +#[cfg(test)] +mod tests { + use std::collections::BTreeSet; + + use crate::compiler::CircuitSimulator; + use acir::{ + acir_field::AcirField, + circuit::{Circuit, ExpressionWidth, Opcode, PublicInputs}, + native_types::{Expression, Witness}, + FieldElement, + }; + + fn test_circuit( + opcodes: Vec>, + private_parameters: BTreeSet, + public_parameters: PublicInputs, + ) -> Circuit { + Circuit { + current_witness_index: 1, + expression_width: ExpressionWidth::Bounded { width: 4 }, + opcodes, + private_parameters, + public_parameters, + return_values: PublicInputs::default(), + assert_messages: Default::default(), + recursive: false, + } + } + + #[test] + fn reports_true_for_empty_circuit() { + let empty_circuit = test_circuit(vec![], BTreeSet::default(), PublicInputs::default()); + + assert!(CircuitSimulator::default().check_circuit(&empty_circuit)); + } + + #[test] + fn reports_true_for_connected_circuit() { + let connected_circuit = test_circuit( + vec![Opcode::AssertZero(Expression { + mul_terms: Vec::new(), + linear_combinations: vec![ + (FieldElement::one(), Witness(1)), + (-FieldElement::one(), Witness(2)), + ], + q_c: FieldElement::zero(), + })], + BTreeSet::from([Witness(1)]), + PublicInputs::default(), + ); + + assert!(CircuitSimulator::default().check_circuit(&connected_circuit)); + } + + #[test] + fn reports_false_for_disconnected_circuit() { + let disconnected_circuit = test_circuit( + vec![ + Opcode::AssertZero(Expression { + mul_terms: Vec::new(), + linear_combinations: vec![ + (FieldElement::one(), Witness(1)), + (-FieldElement::one(), Witness(2)), + ], + q_c: FieldElement::zero(), + }), + Opcode::AssertZero(Expression { + mul_terms: Vec::new(), + linear_combinations: vec![ + (FieldElement::one(), Witness(3)), + (-FieldElement::one(), Witness(4)), + ], + q_c: FieldElement::zero(), + }), + ], + BTreeSet::from([Witness(1)]), + PublicInputs::default(), + ); + + assert!(!CircuitSimulator::default().check_circuit(&disconnected_circuit)); + } +} From 05dd8e3e214bcfb446c29d72531a7b7ed73bff99 Mon Sep 17 00:00:00 2001 From: Tom French Date: Fri, 1 Nov 2024 12:42:06 +0000 Subject: [PATCH 11/11] chore: add tracing for how long we spend inside of the `CircuitSimulator` --- acvm-repo/acvm/src/compiler/simulator.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/acvm-repo/acvm/src/compiler/simulator.rs b/acvm-repo/acvm/src/compiler/simulator.rs index 3a38769a160..d89b53aa564 100644 --- a/acvm-repo/acvm/src/compiler/simulator.rs +++ b/acvm-repo/acvm/src/compiler/simulator.rs @@ -30,8 +30,9 @@ pub struct CircuitSimulator { } impl CircuitSimulator { - // Simulate a symbolic solve for a circuit by keeping track of the witnesses that can be solved. - // Returns false if the circuit cannot be solved + /// Simulate a symbolic solve for a circuit by keeping track of the witnesses that can be solved. + /// Returns false if the circuit cannot be solved + #[tracing::instrument(level = "trace", skip_all)] pub fn check_circuit(&mut self, circuit: &Circuit) -> bool { let circuit_inputs = circuit.circuit_arguments(); self.solvable_witness.extend(circuit_inputs.iter());