From ff9d69b2b25dd8ff7ebc3d4cbfeb6e98d982caa7 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 9 Jun 2023 13:41:03 +0000 Subject: [PATCH 1/4] feat: added support for modulo ops --- crates/nargo_cli/src/cli/compile_cmd.rs | 10 ++- .../brillig_modulo/Nargo.toml | 5 ++ .../brillig_modulo/Prover.toml | 0 .../brillig_modulo/src/main.nr | 28 ++++++++ crates/noirc_evaluator/src/brillig/binary.rs | 38 +++++------ .../src/brillig/brillig_gen.rs | 65 ++++++++++++++++++- 6 files changed, 124 insertions(+), 22 deletions(-) create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/brillig_modulo/Nargo.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/brillig_modulo/Prover.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/brillig_modulo/src/main.nr diff --git a/crates/nargo_cli/src/cli/compile_cmd.rs b/crates/nargo_cli/src/cli/compile_cmd.rs index d2b5532cf24..4812def822c 100644 --- a/crates/nargo_cli/src/cli/compile_cmd.rs +++ b/crates/nargo_cli/src/cli/compile_cmd.rs @@ -2,6 +2,7 @@ use acvm::Backend; use iter_extended::try_vecmap; use nargo::artifacts::contract::PreprocessedContract; use noirc_driver::{CompileOptions, CompiledProgram, Driver}; +use noirc_errors::reporter; use std::path::Path; use clap::Args; @@ -118,5 +119,12 @@ pub(crate) fn compile_circuit( compile_options: &CompileOptions, ) -> Result> { let mut driver = setup_driver(backend, program_dir)?; - driver.compile_main(compile_options).map_err(|_| CliError::CompilationError) + driver.compile_main(compile_options).map_err(|errs| { + let file_manager = driver.file_manager(); + let error_count = reporter::report_all(file_manager, &errs, compile_options.deny_warnings); + if error_count != 0 { + reporter::finish_report(error_count); + } + CliError::CompilationError + }) } diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_modulo/Nargo.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_modulo/Nargo.toml new file mode 100644 index 00000000000..e0b467ce5da --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_modulo/Nargo.toml @@ -0,0 +1,5 @@ +[package] +authors = [""] +compiler_version = "0.1" + +[dependencies] \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_modulo/Prover.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_modulo/Prover.toml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_modulo/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_modulo/src/main.nr new file mode 100644 index 00000000000..1cab78ecb95 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/brillig_modulo/src/main.nr @@ -0,0 +1,28 @@ +// Tests a very simple program. +// +// The features being tested is modulo operations on brillig +fn main() { + assert(modulo(47, 3) == 2); + assert(modulo(2, 3) == 2); + assert(signed_modulo(5, 3) == 2); + assert(signed_modulo(2, 3) == 2); + + let minus_two: i4 = 14; + let minus_three: i4 = 13; + let minus_five: i4 = 11; + + // (5 / -3) * -3 + 2 = -1 * -3 + 2 = 3 + 2 = 5 + assert(signed_modulo(5, minus_three) == 2); + // (-5 / 3) * 3 - 2 = -1 * 3 - 2 = -3 - 2 = -5 + assert(signed_modulo(minus_five, 3) == minus_two); + // (-5 / -3) * -3 - 2 = 1 * -3 - 2 = -3 - 2 = -5 + assert(signed_modulo(minus_five, minus_three) == minus_two); +} + +unconstrained fn modulo(x: u32, y: u32) -> u32 { + x % y +} + +unconstrained fn signed_modulo(x: i4, y: i4) -> i4 { + x % y +} diff --git a/crates/noirc_evaluator/src/brillig/binary.rs b/crates/noirc_evaluator/src/brillig/binary.rs index 93b307e1b70..56f75b70f11 100644 --- a/crates/noirc_evaluator/src/brillig/binary.rs +++ b/crates/noirc_evaluator/src/brillig/binary.rs @@ -44,25 +44,25 @@ impl BrilligBinaryOp { } fn binary_op_to_int_op(op: BinaryOp, is_signed: bool) -> BinaryIntOp { match op { - BinaryOp::Add => BinaryIntOp::Add, - BinaryOp::Sub => BinaryIntOp::Sub, - BinaryOp::Mul => BinaryIntOp::Mul, - BinaryOp::Div => { - if is_signed { - BinaryIntOp::SignedDiv - } else { - BinaryIntOp::UnsignedDiv - } - }, - BinaryOp::Mod => todo!("This is not supported by Brillig. It should either be added into Brillig or legalized by the SSA IR"), - BinaryOp::Eq => BinaryIntOp::Equals, - BinaryOp::Lt => BinaryIntOp::LessThan, - BinaryOp::And => BinaryIntOp::And, - BinaryOp::Or => BinaryIntOp::Or, - BinaryOp::Xor => BinaryIntOp::Xor, - BinaryOp::Shl => BinaryIntOp::Shl, - BinaryOp::Shr => BinaryIntOp::Shr, - } + BinaryOp::Add => BinaryIntOp::Add, + BinaryOp::Sub => BinaryIntOp::Sub, + BinaryOp::Mul => BinaryIntOp::Mul, + BinaryOp::Div => { + if is_signed { + BinaryIntOp::SignedDiv + } else { + BinaryIntOp::UnsignedDiv + } + } + BinaryOp::Mod => unreachable!("Modulo operations are handled separately"), + BinaryOp::Eq => BinaryIntOp::Equals, + BinaryOp::Lt => BinaryIntOp::LessThan, + BinaryOp::And => BinaryIntOp::And, + BinaryOp::Or => BinaryIntOp::Or, + BinaryOp::Xor => BinaryIntOp::Xor, + BinaryOp::Shl => BinaryIntOp::Shl, + BinaryOp::Shr => BinaryIntOp::Shr, + } } // If bit size is available then it is a binary integer operation match bit_size_signedness { diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen.rs b/crates/noirc_evaluator/src/brillig/brillig_gen.rs index 519e341b2be..8f516b66541 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen.rs @@ -7,9 +7,9 @@ use crate::ssa_refactor::ir::{ basic_block::{BasicBlock, BasicBlockId}, dfg::DataFlowGraph, function::Function, - instruction::{Binary, Instruction, InstructionId, TerminatorInstruction}, + instruction::{Binary, BinaryOp, Instruction, InstructionId, TerminatorInstruction}, post_order::PostOrder, - types::Type, + types::{NumericType, Type}, value::{Value, ValueId}, }; use acvm::{ @@ -241,6 +241,20 @@ impl BrilligGen { let left = self.convert_ssa_value(binary.lhs, dfg); let right = self.convert_ssa_value(binary.rhs, dfg); + if let BinaryOp::Mod = binary.operator { + match binary_type { + Type::Numeric(NumericType::Unsigned { bit_size }) => { + self.convert_integer_mod(result_register, left, right, bit_size, false); + return; + } + Type::Numeric(NumericType::Signed { bit_size }) => { + self.convert_integer_mod(result_register, left, right, bit_size, true); + return; + } + _ => unimplemented!("ICE: Modulo operation not supported for type {binary_type:?}"), + } + } + let brillig_binary_op = BrilligBinaryOp::convert_ssa_binary_op_to_brillig_binary_op( binary.operator, binary_type, @@ -268,6 +282,53 @@ impl BrilligGen { } } + /// Issues a modulo operation on integers. + /// This is done by first dividing the left operand by the right operand, + /// then multiplying the result by the right operand, and finally subtracting + /// the result from the left operand. + /// result = left - ( (left / right) * right) + fn convert_integer_mod( + &mut self, + result_register: RegisterIndex, + left: RegisterIndex, + right: RegisterIndex, + bit_size: u32, + signed: bool, + ) { + let scratch_register_i = self.create_register(); + let scratch_register_j = self.create_register(); + + // i = left / right + self.push_code(BrilligOpcode::BinaryIntOp { + op: match signed { + true => BinaryIntOp::SignedDiv, + false => BinaryIntOp::UnsignedDiv, + }, + destination: scratch_register_i, + bit_size, + lhs: left, + rhs: right, + }); + + // j = i * right + self.push_code(BrilligOpcode::BinaryIntOp { + op: BinaryIntOp::Mul, + destination: scratch_register_j, + bit_size, + lhs: scratch_register_i, + rhs: right, + }); + + // result_register = left - j + self.push_code(BrilligOpcode::BinaryIntOp { + op: BinaryIntOp::Sub, + destination: result_register, + bit_size, + lhs: left, + rhs: scratch_register_j, + }); + } + /// Converts an SSA `ValueId` into a `RegisterIndex`. fn convert_ssa_value(&mut self, value_id: ValueId, dfg: &DataFlowGraph) -> RegisterIndex { let value = &dfg[value_id]; From b004ac1d861521357d56797d23f6112217e00cd8 Mon Sep 17 00:00:00 2001 From: sirasistant Date: Fri, 9 Jun 2023 15:11:56 +0000 Subject: [PATCH 2/4] chore: use ACVM 0.14.3 --- Cargo.lock | 20 ++++++++++---------- Cargo.toml | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 90260cc3009..86d0a9cf8fc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4,9 +4,9 @@ version = 3 [[package]] name = "acir" -version = "0.14.2" +version = "0.14.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cafbc2adec7783509c6e06831ef57e672fc89c963c855a5fe0449c1ebb2822bc" +checksum = "5f2d2b80c4e6e0c6f2b5d45693925f6eac1e6fcf9a908aad70158e58b9ed18b0" dependencies = [ "acir_field", "brillig_vm", @@ -18,9 +18,9 @@ dependencies = [ [[package]] name = "acir_field" -version = "0.14.2" +version = "0.14.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6a33855c911b77f0eddd9e0a6b9410238a5ba80a4956dcf335dd65dd4d2303d6" +checksum = "c984be877eae94755269391ebde859d65416c7aa7955badc3338464dbbdd1f38" dependencies = [ "ark-bn254", "ark-ff", @@ -32,9 +32,9 @@ dependencies = [ [[package]] name = "acvm" -version = "0.14.2" +version = "0.14.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0ad7d5020f3cee249e162504deeeb1a83fa2a727e4c6a5277136bdecbe82cf2b" +checksum = "f31caccd81f7ea1ce5a9417ff283da0f4932ee28d63f6554049de03fe3d69d77" dependencies = [ "acir", "acvm_stdlib", @@ -71,9 +71,9 @@ dependencies = [ [[package]] name = "acvm_stdlib" -version = "0.14.2" +version = "0.14.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4c9bf39b1e3b486f090ca1c0ef1e7d47fabfdb522af58117d1d86de048ac0571" +checksum = "b42f48d5c9ecfb1662ec7cbcf337fb4bb9323daf6e0168574431a9ec29684beb" dependencies = [ "acir", ] @@ -514,9 +514,9 @@ dependencies = [ [[package]] name = "brillig_vm" -version = "0.14.2" +version = "0.14.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a577e930e991623dd1ec5f9540b20eb601d06ae9d7f66181b81ff699441fc159" +checksum = "331e59215d1d45d136bcba2f1bbf18cf6c93477375afb851c4afaf8d7aa21b27" dependencies = [ "acir_field", "num-bigint", diff --git a/Cargo.toml b/Cargo.toml index 38231e4bb24..72469ca2e20 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,7 +24,7 @@ edition = "2021" rust-version = "1.66" [workspace.dependencies] -acvm = "=0.14.2" +acvm = "=0.14.3" arena = { path = "crates/arena" } fm = { path = "crates/fm" } iter-extended = { path = "crates/iter-extended" } From 4c92dc55e6b0beedea83b94da4a4367c0a3b28d6 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Fri, 9 Jun 2023 18:12:57 +0000 Subject: [PATCH 3/4] revert change as its unrelated --- crates/nargo_cli/src/cli/compile_cmd.rs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/crates/nargo_cli/src/cli/compile_cmd.rs b/crates/nargo_cli/src/cli/compile_cmd.rs index 4812def822c..d2b5532cf24 100644 --- a/crates/nargo_cli/src/cli/compile_cmd.rs +++ b/crates/nargo_cli/src/cli/compile_cmd.rs @@ -2,7 +2,6 @@ use acvm::Backend; use iter_extended::try_vecmap; use nargo::artifacts::contract::PreprocessedContract; use noirc_driver::{CompileOptions, CompiledProgram, Driver}; -use noirc_errors::reporter; use std::path::Path; use clap::Args; @@ -119,12 +118,5 @@ pub(crate) fn compile_circuit( compile_options: &CompileOptions, ) -> Result> { let mut driver = setup_driver(backend, program_dir)?; - driver.compile_main(compile_options).map_err(|errs| { - let file_manager = driver.file_manager(); - let error_count = reporter::report_all(file_manager, &errs, compile_options.deny_warnings); - if error_count != 0 { - reporter::finish_report(error_count); - } - CliError::CompilationError - }) + driver.compile_main(compile_options).map_err(|_| CliError::CompilationError) } From df0a630fe75ac9e2f59ac26b964f08fae64e50ca Mon Sep 17 00:00:00 2001 From: kevaundray Date: Fri, 9 Jun 2023 18:21:32 +0000 Subject: [PATCH 4/4] small comment refactor --- crates/noirc_evaluator/src/brillig/brillig_gen.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen.rs b/crates/noirc_evaluator/src/brillig/brillig_gen.rs index 8f516b66541..07e9adb00ac 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen.rs @@ -241,6 +241,9 @@ impl BrilligGen { let left = self.convert_ssa_value(binary.lhs, dfg); let right = self.convert_ssa_value(binary.rhs, dfg); + // Process modulo operator separately as there is no + // Brillig modulo operator and the result is multiple + // brillig opcodes. if let BinaryOp::Mod = binary.operator { match binary_type { Type::Numeric(NumericType::Unsigned { bit_size }) => { @@ -282,11 +285,11 @@ impl BrilligGen { } } - /// Issues a modulo operation on integers. - /// This is done by first dividing the left operand by the right operand, - /// then multiplying the result by the right operand, and finally subtracting - /// the result from the left operand. - /// result = left - ( (left / right) * right) + /// Computes left % right by emitting the necessary Brillig opcodes. + /// + /// This is done by using the following formula: + /// + /// a % b = a - (b * (a / b)) fn convert_integer_mod( &mut self, result_register: RegisterIndex,