From 70acb4c5326d63e0280325a79b7c5e7fdfc7ae9a Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Wed, 31 May 2023 21:21:55 +0100 Subject: [PATCH 1/7] add shift left and shift right in AcirVar --- .../acir_gen/acir_ir/acir_variable.rs | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index 91fd17990e9..9c45fa94778 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -265,6 +265,54 @@ impl AcirContext { } } } + /// Returns an `AcirVar` that is constrained to be lhs << rhs. + /// + /// We convert left shifts to multiplications, so this is equivalent to + /// lhs * 2^rhs. + /// + /// TODO: Currently, we maintain that the rhs needs to be a constant so that we can + /// TODO: compute 2^rhs in the compiler. However, we can extend it to witnesses + /// TODO: by first bit decomposing rhs and then using square-and-multiply to + /// TODO: compute 2^{rhs}. This will require if statements so it would be good + /// TODO: to have this implementation in Noir and the << operator uses that. + pub(crate) fn shift_left_var(&mut self, lhs: AcirVar, rhs: AcirVar) -> AcirVar { + let rhs_data = &self.data[&rhs]; + + // Compute 2^{rhs} + let two_pow_rhs = match rhs_data.as_constant() { + Some(exponent) => FieldElement::from(2_i128).pow(&exponent), + None => unimplemented!("rhs must be a constant when doing a right shift"), + }; + let two_pow_rhs_var = self.add_constant(two_pow_rhs); + + self.mul_var(lhs, two_pow_rhs_var) + } + + /// Returns an `AcirVar` that is constrained to be lhs >> rhs. + /// + /// We convert right shifts to divisions, so this is equivalent to + /// lhs / 2^rhs. + /// + /// TODO: the previous code seems to have only done a right shift, if the + /// TODO rhs was a constant. Moreover, it did an unsigned division. + /// + /// TODO: We should rationalize whether we want shift left and shift right to work + /// TODO only for unsigned integers. + /// TODO: + /// TODO: This would mean that both implementation would be doing integer mul and + /// TODO integer division. The previous code was doing Mul and integer division. + pub(crate) fn shift_right_var(&mut self, lhs: AcirVar, rhs: AcirVar) -> AcirVar { + let rhs_data = &self.data[&rhs]; + + // Compute 2^{rhs} + let two_pow_rhs = match rhs_data.as_constant() { + Some(exponent) => FieldElement::from(2_i128).pow(&exponent), + None => unimplemented!("rhs must be a constant when doing a right shift"), + }; + let two_pow_rhs_var = self.add_constant(two_pow_rhs); + + self.div_var(lhs, two_pow_rhs_var) + } /// Converts the `AcirVar` to a `Witness` if it hasn't been already, and appends it to the /// `GeneratedAcir`'s return witnesses. From f77cdd7bece6008080b9e848e44ee26428cbe72a Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Wed, 31 May 2023 21:22:12 +0100 Subject: [PATCH 2/7] call shl and shr in `convert_ssa_binary` --- crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs index bde31572ff7..cdcc1eec776 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -197,6 +197,8 @@ impl Context { .acir_context .less_than_var(lhs, rhs) .expect("add Result types to all methods so errors bubble up"), + BinaryOp::Shl => self.acir_context.shift_left_var(lhs, rhs), + BinaryOp::Shr => self.acir_context.shift_right_var(lhs, rhs), _ => todo!(), } } From 0d8e08e60d5718f3be40deaa13c36a014ea1470d Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Wed, 31 May 2023 21:24:25 +0100 Subject: [PATCH 3/7] add example --- .../simple_shift_left_right/Nargo.toml | 5 +++++ .../simple_shift_left_right/Prover.toml | 1 + .../simple_shift_left_right/src/main.nr | 8 ++++++++ 3 files changed, 14 insertions(+) create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/simple_shift_left_right/Nargo.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/simple_shift_left_right/Prover.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/simple_shift_left_right/src/main.nr diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/simple_shift_left_right/Nargo.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/simple_shift_left_right/Nargo.toml new file mode 100644 index 00000000000..e0b467ce5da --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/simple_shift_left_right/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/simple_shift_left_right/Prover.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/simple_shift_left_right/Prover.toml new file mode 100644 index 00000000000..07890234a19 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/simple_shift_left_right/Prover.toml @@ -0,0 +1 @@ +x = "3" diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/simple_shift_left_right/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/simple_shift_left_right/src/main.nr new file mode 100644 index 00000000000..1bcbbde2eb5 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/simple_shift_left_right/src/main.nr @@ -0,0 +1,8 @@ +// Tests a very simple program. +// +// The features being tested are left and right shifts. +fn main(x : u32) { + let z = x >> 4; + let t = x << 4; + assert(z == t >> 8); +} \ No newline at end of file From 4221aeecc9c06364fd7ed84806f558f367e9eba1 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 1 Jun 2023 12:25:31 +0100 Subject: [PATCH 4/7] remove TODO --- .../src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index 49d9bc911bd..5a5b8dd9381 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -274,13 +274,8 @@ impl AcirContext { /// Returns an `AcirVar` that is constrained to be lhs << rhs. /// /// We convert left shifts to multiplications, so this is equivalent to - /// lhs * 2^rhs. - /// - /// TODO: Currently, we maintain that the rhs needs to be a constant so that we can - /// TODO: compute 2^rhs in the compiler. However, we can extend it to witnesses - /// TODO: by first bit decomposing rhs and then using square-and-multiply to - /// TODO: compute 2^{rhs}. This will require if statements so it would be good - /// TODO: to have this implementation in Noir and the << operator uses that. + /// lhs * 2^rhs. We currently maintain that rhs needs to be a constant + /// however this can be extended, see #1478. pub(crate) fn shift_left_var(&mut self, lhs: AcirVar, rhs: AcirVar) -> AcirVar { let rhs_data = &self.data[&rhs]; From 7db1b4508686ec070b990b2893d747374bc526d8 Mon Sep 17 00:00:00 2001 From: Kevaundray Wedderburn Date: Thu, 1 Jun 2023 12:31:34 +0100 Subject: [PATCH 5/7] update todos --- .../acir_gen/acir_ir/acir_variable.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index 5a5b8dd9381..8e0bdbfe2cd 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -274,7 +274,9 @@ impl AcirContext { /// Returns an `AcirVar` that is constrained to be lhs << rhs. /// /// We convert left shifts to multiplications, so this is equivalent to - /// lhs * 2^rhs. We currently maintain that rhs needs to be a constant + /// lhs * 2^rhs. + /// + /// We currently maintain that rhs needs to be a constant /// however this can be extended, see #1478. pub(crate) fn shift_left_var(&mut self, lhs: AcirVar, rhs: AcirVar) -> AcirVar { let rhs_data = &self.data[&rhs]; @@ -294,14 +296,11 @@ impl AcirContext { /// We convert right shifts to divisions, so this is equivalent to /// lhs / 2^rhs. /// - /// TODO: the previous code seems to have only done a right shift, if the - /// TODO rhs was a constant. Moreover, it did an unsigned division. + /// We currently maintain that rhs needs to be a constant + /// however this can be extended, see #1478. /// - /// TODO: We should rationalize whether we want shift left and shift right to work - /// TODO only for unsigned integers. - /// TODO: - /// TODO: This would mean that both implementation would be doing integer mul and - /// TODO integer division. The previous code was doing Mul and integer division. + /// This code is doing a field division instead of an integer division, + /// see #1479 about how this is expected to change. pub(crate) fn shift_right_var(&mut self, lhs: AcirVar, rhs: AcirVar) -> AcirVar { let rhs_data = &self.data[&rhs]; From b57e3479a4f4ee78559d440f11e01c0cc84f4976 Mon Sep 17 00:00:00 2001 From: kevaundray Date: Thu, 1 Jun 2023 13:07:11 +0100 Subject: [PATCH 6/7] Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index 8e0bdbfe2cd..83d84f4dc10 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -271,12 +271,13 @@ impl AcirContext { } } } - /// Returns an `AcirVar` that is constrained to be lhs << rhs. + + /// Returns an `AcirVar` that is constrained to be `lhs << rhs`. /// /// We convert left shifts to multiplications, so this is equivalent to - /// lhs * 2^rhs. + /// `lhs * 2^rhs`. /// - /// We currently maintain that rhs needs to be a constant + /// We currently require `rhs` to be a constant /// however this can be extended, see #1478. pub(crate) fn shift_left_var(&mut self, lhs: AcirVar, rhs: AcirVar) -> AcirVar { let rhs_data = &self.data[&rhs]; From 4e18d3c02d974f2b66b77452ef99b6e07b4df6a6 Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 1 Jun 2023 09:01:31 -0500 Subject: [PATCH 7/7] Update crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- .../src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs index 83d84f4dc10..59f1a1f7010 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/acir_ir/acir_variable.rs @@ -292,12 +292,12 @@ impl AcirContext { self.mul_var(lhs, two_pow_rhs_var) } - /// Returns an `AcirVar` that is constrained to be lhs >> rhs. + /// Returns an `AcirVar` that is constrained to be `lhs >> rhs`. /// /// We convert right shifts to divisions, so this is equivalent to - /// lhs / 2^rhs. + /// `lhs / 2^rhs`. /// - /// We currently maintain that rhs needs to be a constant + /// We currently require `rhs` to be a constant /// however this can be extended, see #1478. /// /// This code is doing a field division instead of an integer division,