From 2d7edff6c6228da7b8f59ccac22962a6023fd3cb Mon Sep 17 00:00:00 2001 From: guipublic Date: Thu, 27 Jul 2023 17:03:04 +0000 Subject: [PATCH 1/8] remove shr and shl from ssa instruction --- .../bit_shifts_runtime/Nargo.toml | 5 ++ .../bit_shifts_runtime/Prover.toml | 2 + .../bit_shifts_runtime/src/main.nr | 9 +++ .../src/brillig/brillig_gen/brillig_block.rs | 2 - .../src/brillig/brillig_ir/debug_show.rs | 5 +- .../src/ssa_refactor/acir_gen/mod.rs | 7 -- .../src/ssa_refactor/ir/instruction.rs | 20 ----- .../src/ssa_refactor/ssa_gen/context.rs | 77 +++++++++++++++---- 8 files changed, 80 insertions(+), 47 deletions(-) create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/Nargo.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/Prover.toml create mode 100644 crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/src/main.nr diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/Nargo.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/Nargo.toml new file mode 100644 index 00000000000..e0b467ce5da --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/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/bit_shifts_runtime/Prover.toml b/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/Prover.toml new file mode 100644 index 00000000000..67bf6a6a234 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/Prover.toml @@ -0,0 +1,2 @@ +x = 64 +y = 1 diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/src/main.nr b/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/src/main.nr new file mode 100644 index 00000000000..271a1ecb880 --- /dev/null +++ b/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/src/main.nr @@ -0,0 +1,9 @@ +fn main(x: u64, y: u64) { + // runtime shifts on comptime values + assert(64 << y == 128); + assert(64 >> y == 32); + + // runtime shifts on runtime values + assert(x << y == 128); + assert(x >> y == 32); +} \ No newline at end of file diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 4de052aad9d..44a8c52a056 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -966,8 +966,6 @@ pub(crate) fn convert_ssa_binary_op_to_brillig_binary_op( BinaryOp::And => BinaryIntOp::And, BinaryOp::Or => BinaryIntOp::Or, BinaryOp::Xor => BinaryIntOp::Xor, - BinaryOp::Shl => BinaryIntOp::Shl, - BinaryOp::Shr => BinaryIntOp::Shr, }; BrilligBinaryOp::Integer { op: operation, bit_size } diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs b/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs index 75716e49177..26df8d31c93 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs @@ -73,8 +73,9 @@ impl DebugToString for BinaryIntOp { BinaryIntOp::And => "&&".into(), BinaryIntOp::Or => "||".into(), BinaryIntOp::Xor => "^".into(), - BinaryIntOp::Shl => "<<".into(), - BinaryIntOp::Shr => ">>".into(), + BinaryIntOp::Shl | BinaryIntOp::Shr => { + unreachable!("bit shift should haver been replaced") + } } } } 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 b0ade9419fe..08e756ac4c6 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/acir_gen/mod.rs @@ -733,13 +733,6 @@ impl Context { bit_count, self.current_side_effects_enabled_var, ), - BinaryOp::Shl => self.acir_context.shift_left_var(lhs, rhs, binary_type), - BinaryOp::Shr => self.acir_context.shift_right_var( - lhs, - rhs, - binary_type, - self.current_side_effects_enabled_var, - ), BinaryOp::Xor => self.acir_context.xor_var(lhs, rhs, binary_type), BinaryOp::And => self.acir_context.and_var(lhs, rhs, binary_type), BinaryOp::Or => self.acir_context.or_var(lhs, rhs, binary_type), diff --git a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs index 416c53ba6b4..7500b3ee17d 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ir/instruction.rs @@ -752,16 +752,6 @@ impl Binary { return SimplifyResult::SimplifiedTo(zero); } } - BinaryOp::Shl => { - if rhs_is_zero { - return SimplifyResult::SimplifiedTo(self.lhs); - } - } - BinaryOp::Shr => { - if rhs_is_zero { - return SimplifyResult::SimplifiedTo(self.lhs); - } - } } SimplifyResult::None } @@ -817,8 +807,6 @@ impl BinaryOp { BinaryOp::And => None, BinaryOp::Or => None, BinaryOp::Xor => None, - BinaryOp::Shl => None, - BinaryOp::Shr => None, } } @@ -832,8 +820,6 @@ impl BinaryOp { BinaryOp::And => |x, y| Some(x & y), BinaryOp::Or => |x, y| Some(x | y), BinaryOp::Xor => |x, y| Some(x ^ y), - BinaryOp::Shl => |x, y| x.checked_shl(y.try_into().ok()?), - BinaryOp::Shr => |x, y| Some(x >> y), BinaryOp::Eq => |x, y| Some((x == y) as u128), BinaryOp::Lt => |x, y| Some((x < y) as u128), } @@ -874,10 +860,6 @@ pub(crate) enum BinaryOp { Or, /// Bitwise xor (^) Xor, - /// Shift lhs left by rhs bits (<<) - Shl, - /// Shift lhs right by rhs bits (>>) - Shr, } impl std::fmt::Display for BinaryOp { @@ -893,8 +875,6 @@ impl std::fmt::Display for BinaryOp { BinaryOp::And => write!(f, "and"), BinaryOp::Or => write!(f, "or"), BinaryOp::Xor => write!(f, "xor"), - BinaryOp::Shl => write!(f, "shl"), - BinaryOp::Shr => write!(f, "shr"), } } } diff --git a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs index 769ee6aa09f..821e1197f3d 100644 --- a/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs +++ b/crates/noirc_evaluator/src/ssa_refactor/ssa_gen/context.rs @@ -7,12 +7,12 @@ use iter_extended::vecmap; use noirc_errors::Location; use noirc_frontend::monomorphization::ast::{self, LocalId, Parameters}; use noirc_frontend::monomorphization::ast::{FuncId, Program}; -use noirc_frontend::Signedness; +use noirc_frontend::{BinaryOpKind, Signedness}; use crate::ssa_refactor::ir::dfg::DataFlowGraph; use crate::ssa_refactor::ir::function::FunctionId as IrFunctionId; use crate::ssa_refactor::ir::function::{Function, RuntimeType}; -use crate::ssa_refactor::ir::instruction::BinaryOp; +use crate::ssa_refactor::ir::instruction::{BinaryOp, Endian, Intrinsic}; use crate::ssa_refactor::ir::map::AtomicCounter; use crate::ssa_refactor::ir::types::{NumericType, Type}; use crate::ssa_refactor::ir::value::ValueId; @@ -224,6 +224,46 @@ impl<'a> FunctionContext<'a> { Values::empty() } + /// Insert ssa instructions which computes lhs << rhs by doing lhs*2^rhs + fn insert_shift_left(&mut self, lhs: ValueId, rhs: ValueId) -> ValueId { + let base = self.builder.field_constant(FieldElement::from(2_u128)); + let pow = self.pow(base, rhs); + self.builder.insert_binary(lhs, BinaryOp::Mul, pow) + } + + /// Insert ssa instructions which computes lhs << rhs by doing lhs/2^rhs + fn insert_shift_right(&mut self, lhs: ValueId, rhs: ValueId) -> ValueId { + let base = self.builder.field_constant(FieldElement::from(2_u128)); + let pow = self.pow(base, rhs); + self.builder.insert_binary(lhs, BinaryOp::Div, pow) + } + + /// Computes lhs^rhs via square&multiply, using the bits decomposition of rhs + fn pow(&mut self, lhs: ValueId, rhs: ValueId) -> ValueId { + let typ = self.builder.current_function.dfg.type_of_value(rhs); + if let Type::Numeric(NumericType::Unsigned { bit_size }) = typ { + let to_bits = self.builder.import_intrinsic_id(Intrinsic::ToBits(Endian::Little)); + let length = self.builder.field_constant(FieldElement::from(bit_size as i128)); + let result_types = vec![Type::Array(Rc::new(vec![Type::bool()]), bit_size as usize)]; + let rhs_bits = self.builder.insert_call(to_bits, vec![rhs, length], result_types)[0]; + let one = self.builder.field_constant(FieldElement::one()); + let mut r = one; + for i in 1..bit_size + 1 { + let r1 = self.builder.insert_binary(r, BinaryOp::Mul, r); + let a = self.builder.insert_binary(r1, BinaryOp::Mul, lhs); + let idx = self.builder.field_constant(FieldElement::from((bit_size - i) as i128)); + let b = self.builder.insert_array_get(rhs_bits, idx, Type::field()); + let r2 = self.builder.insert_binary(a, BinaryOp::Mul, b); + let c = self.builder.insert_binary(one, BinaryOp::Sub, b); + let r3 = self.builder.insert_binary(c, BinaryOp::Mul, r1); + r = self.builder.insert_binary(r2, BinaryOp::Add, r3); + } + r + } else { + unreachable!("Value must be unsigned in power operation"); + } + } + /// Insert a binary instruction at the end of the current block. /// Converts the form of the binary instruction as necessary /// (e.g. swapping arguments, inserting a not) to represent it in the IR. @@ -235,17 +275,22 @@ impl<'a> FunctionContext<'a> { mut rhs: ValueId, location: Location, ) -> Values { - let op = convert_operator(operator); - - if op == BinaryOp::Eq && matches!(self.builder.type_of_value(lhs), Type::Array(..)) { - return self.insert_array_equality(lhs, operator, rhs, location); - } - - if operator_requires_swapped_operands(operator) { - std::mem::swap(&mut lhs, &mut rhs); - } - - let mut result = self.builder.set_location(location).insert_binary(lhs, op, rhs); + let mut result = match operator { + BinaryOpKind::ShiftLeft => self.insert_shift_left(lhs, rhs), + BinaryOpKind::ShiftRight => self.insert_shift_right(lhs, rhs), + BinaryOpKind::Equal | BinaryOpKind::NotEqual + if matches!(self.builder.type_of_value(lhs), Type::Array(..)) => + { + return self.insert_array_equality(lhs, operator, rhs, location) + } + _ => { + let op = convert_operator(operator); + if operator_requires_swapped_operands(operator) { + std::mem::swap(&mut lhs, &mut rhs); + } + self.builder.set_location(location).insert_binary(lhs, op, rhs) + } + }; if let Some(max_bit_size) = operator_result_max_bit_size_to_truncate( operator, @@ -692,7 +737,6 @@ fn operator_result_max_bit_size_to_truncate( /// checking operator_requires_not and operator_requires_swapped_operands /// to represent the full operation correctly. fn convert_operator(op: noirc_frontend::BinaryOpKind) -> BinaryOp { - use noirc_frontend::BinaryOpKind; match op { BinaryOpKind::Add => BinaryOp::Add, BinaryOpKind::Subtract => BinaryOp::Sub, @@ -708,8 +752,9 @@ fn convert_operator(op: noirc_frontend::BinaryOpKind) -> BinaryOp { BinaryOpKind::And => BinaryOp::And, BinaryOpKind::Or => BinaryOp::Or, BinaryOpKind::Xor => BinaryOp::Xor, - BinaryOpKind::ShiftRight => BinaryOp::Shr, - BinaryOpKind::ShiftLeft => BinaryOp::Shl, + BinaryOpKind::ShiftRight | BinaryOpKind::ShiftLeft => unreachable!( + "ICE - bit shift operators do not exist in SSA and should have been replaced" + ), } } From 4c33c3cbebb9f45326c33f93debf8f97f99581bf Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 28 Jul 2023 09:24:55 +0000 Subject: [PATCH 2/8] move bit_shift_runtime test to test_data --- .../bit_shifts_runtime/Nargo.toml | 3 ++- .../bit_shifts_runtime/Prover.toml | 2 +- .../bit_shifts_runtime/src/main.nr | 0 3 files changed, 3 insertions(+), 2 deletions(-) rename crates/nargo_cli/tests/{test_data_ssa_refactor => test_data}/bit_shifts_runtime/Nargo.toml (54%) rename crates/nargo_cli/tests/{test_data_ssa_refactor => test_data}/bit_shifts_runtime/Prover.toml (53%) rename crates/nargo_cli/tests/{test_data_ssa_refactor => test_data}/bit_shifts_runtime/src/main.nr (100%) diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/Nargo.toml b/crates/nargo_cli/tests/test_data/bit_shifts_runtime/Nargo.toml similarity index 54% rename from crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/Nargo.toml rename to crates/nargo_cli/tests/test_data/bit_shifts_runtime/Nargo.toml index e0b467ce5da..661f4f937d5 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/Nargo.toml +++ b/crates/nargo_cli/tests/test_data/bit_shifts_runtime/Nargo.toml @@ -1,5 +1,6 @@ [package] +name = "bit_shifts_runtime" authors = [""] compiler_version = "0.1" -[dependencies] \ No newline at end of file +[dependencies] diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/Prover.toml b/crates/nargo_cli/tests/test_data/bit_shifts_runtime/Prover.toml similarity index 53% rename from crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/Prover.toml rename to crates/nargo_cli/tests/test_data/bit_shifts_runtime/Prover.toml index 67bf6a6a234..98d8630792e 100644 --- a/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/Prover.toml +++ b/crates/nargo_cli/tests/test_data/bit_shifts_runtime/Prover.toml @@ -1,2 +1,2 @@ x = 64 -y = 1 +y = 1 \ No newline at end of file diff --git a/crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/src/main.nr b/crates/nargo_cli/tests/test_data/bit_shifts_runtime/src/main.nr similarity index 100% rename from crates/nargo_cli/tests/test_data_ssa_refactor/bit_shifts_runtime/src/main.nr rename to crates/nargo_cli/tests/test_data/bit_shifts_runtime/src/main.nr From 7ee4e28db9cc22334d76895b9002457781c0fb52 Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 28 Jul 2023 09:27:19 +0000 Subject: [PATCH 3/8] code review, fix typo --- crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs b/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs index 26df8d31c93..2bb753de760 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir/debug_show.rs @@ -74,7 +74,7 @@ impl DebugToString for BinaryIntOp { BinaryIntOp::Or => "||".into(), BinaryIntOp::Xor => "^".into(), BinaryIntOp::Shl | BinaryIntOp::Shr => { - unreachable!("bit shift should haver been replaced") + unreachable!("bit shift should have been replaced") } } } From f515e6e7ce61b5265b57d59f696853453c83b1a9 Mon Sep 17 00:00:00 2001 From: guipublic Date: Fri, 28 Jul 2023 14:47:19 +0000 Subject: [PATCH 4/8] Forbid signed integers for bit shift and fix brillig failing test --- .../src/brillig/brillig_gen/brillig_block.rs | 22 +++++++++---------- .../src/brillig/brillig_gen/brillig_fn.rs | 7 ------ .../noirc_evaluator/src/brillig/brillig_ir.rs | 12 ++++++++++ .../noirc_frontend/src/hir/type_check/expr.rs | 12 +++++++--- crates/noirc_frontend/src/hir_def/expr.rs | 5 +++++ 5 files changed, 37 insertions(+), 21 deletions(-) diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 44a8c52a056..f01551fcf90 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -336,10 +336,10 @@ impl<'block> BrilligBlock<'block> { dfg.instruction_results(instruction_id)[0], dfg, ); - + let heap_vec = self.brillig_context.extract_heap_vector(target_slice); self.brillig_context.radix_instruction( source, - self.function_context.extract_heap_vector(target_slice), + heap_vec, radix, limb_count, matches!(endianness, Endian::Big), @@ -355,10 +355,10 @@ impl<'block> BrilligBlock<'block> { ); let radix = self.brillig_context.make_constant(2_usize.into()); - + let heap_vec = self.brillig_context.extract_heap_vector(target_slice); self.brillig_context.radix_instruction( source, - self.function_context.extract_heap_vector(target_slice), + heap_vec, radix, limb_count, matches!(endianness, Endian::Big), @@ -589,7 +589,7 @@ impl<'block> BrilligBlock<'block> { dfg.instruction_results(instruction_id)[0], dfg, ); - let target_vector = self.function_context.extract_heap_vector(target_variable); + let target_vector = self.brillig_context.extract_heap_vector(target_variable); let item_value = self.convert_ssa_register_value(arguments[1], dfg); slice_push_back_operation( self.brillig_context, @@ -604,7 +604,7 @@ impl<'block> BrilligBlock<'block> { dfg.instruction_results(instruction_id)[0], dfg, ); - let target_vector = self.function_context.extract_heap_vector(target_variable); + let target_vector = self.brillig_context.extract_heap_vector(target_variable); let item_value = self.convert_ssa_register_value(arguments[1], dfg); slice_push_front_operation( self.brillig_context, @@ -618,7 +618,7 @@ impl<'block> BrilligBlock<'block> { let target_variable = self.function_context.create_variable(self.brillig_context, results[0], dfg); - let target_vector = self.function_context.extract_heap_vector(target_variable); + let target_vector = self.brillig_context.extract_heap_vector(target_variable); let pop_item = self.function_context.create_register_variable( self.brillig_context, @@ -643,7 +643,7 @@ impl<'block> BrilligBlock<'block> { ); let target_variable = self.function_context.create_variable(self.brillig_context, results[1], dfg); - let target_vector = self.function_context.extract_heap_vector(target_variable); + let target_vector = self.brillig_context.extract_heap_vector(target_variable); slice_pop_front_operation( self.brillig_context, @@ -659,7 +659,7 @@ impl<'block> BrilligBlock<'block> { let target_variable = self.function_context.create_variable(self.brillig_context, results[0], dfg); - let target_vector = self.function_context.extract_heap_vector(target_variable); + let target_vector = self.brillig_context.extract_heap_vector(target_variable); slice_insert_operation( self.brillig_context, target_vector, @@ -674,7 +674,7 @@ impl<'block> BrilligBlock<'block> { let target_variable = self.function_context.create_variable(self.brillig_context, results[0], dfg); - let target_vector = self.function_context.extract_heap_vector(target_variable); + let target_vector = self.brillig_context.extract_heap_vector(target_variable); let removed_item_register = self.function_context.create_register_variable( self.brillig_context, @@ -862,7 +862,7 @@ impl<'block> BrilligBlock<'block> { Type::Slice(_) => { let variable = self.function_context.create_variable(self.brillig_context, result, dfg); - let vector = self.function_context.extract_heap_vector(variable); + let vector = self.brillig_context.extract_heap_vector(variable); // Set the pointer to the current stack frame // The stack pointer will then be update by the caller of this method diff --git a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs index 1a751d28b23..210d6da7be6 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_gen/brillig_fn.rs @@ -115,13 +115,6 @@ impl FunctionContext { } } - pub(crate) fn extract_heap_vector(&self, variable: RegisterOrMemory) -> HeapVector { - match variable { - RegisterOrMemory::HeapVector(vector) => vector, - _ => unreachable!("ICE: Expected vector, got {variable:?}"), - } - } - /// Collects the registers that a given variable is stored in. pub(crate) fn extract_registers(&self, variable: RegisterOrMemory) -> Vec { match variable { diff --git a/crates/noirc_evaluator/src/brillig/brillig_ir.rs b/crates/noirc_evaluator/src/brillig/brillig_ir.rs index ac0103dd9ed..4471d507579 100644 --- a/crates/noirc_evaluator/src/brillig/brillig_ir.rs +++ b/crates/noirc_evaluator/src/brillig/brillig_ir.rs @@ -951,6 +951,18 @@ impl BrilligContext { self.deallocate_register(end_value_register); self.deallocate_register(index_at_end_of_array); } + + pub(crate) fn extract_heap_vector(&mut self, variable: RegisterOrMemory) -> HeapVector { + match variable { + RegisterOrMemory::HeapVector(vector) => vector, + RegisterOrMemory::HeapArray(array) => { + let size = self.allocate_register(); + self.const_instruction(size, array.size.into()); + HeapVector { pointer: array.pointer, size } + } + _ => unreachable!("ICE: Expected vector, got {variable:?}"), + } + } } /// Type to encapsulate the binary operation types in Brillig diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index a2ff1c23a63..a8b5a52aaf4 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -11,7 +11,7 @@ use crate::{ types::Type, }, node_interner::{ExprId, FuncId}, - CompTime, Shared, TypeBinding, TypeVariableKind, UnaryOp, + CompTime, Shared, Signedness, TypeBinding, TypeVariableKind, UnaryOp, }; use super::{errors::TypeCheckError, TypeChecker}; @@ -916,8 +916,14 @@ impl<'interner> TypeChecker<'interner> { span, }); } - let comptime = comptime_x.and(comptime_y, op.location.span); - Ok(Integer(comptime, *sign_x, *bit_width_x)) + if op.is_bit_shift() + && (*sign_x != Signedness::Unsigned || *sign_y != Signedness::Unsigned) + { + Err(TypeCheckError::InvalidInfixOp { kind: "Signed integer", span }) + } else { + let comptime = comptime_x.and(comptime_y, op.location.span); + Ok(Integer(comptime, *sign_x, *bit_width_x)) + } } (Integer(..), FieldElement(..)) | (FieldElement(..), Integer(..)) => { Err(TypeCheckError::IntegerAndFieldBinaryOperation { span }) diff --git a/crates/noirc_frontend/src/hir_def/expr.rs b/crates/noirc_frontend/src/hir_def/expr.rs index 63b7e421dc3..5183cd44d9f 100644 --- a/crates/noirc_frontend/src/hir_def/expr.rs +++ b/crates/noirc_frontend/src/hir_def/expr.rs @@ -72,6 +72,11 @@ impl HirBinaryOp { use BinaryOpKind::*; matches!(self.kind, And | Or | Xor | ShiftRight | ShiftLeft) } + + pub fn is_bit_shift(&self) -> bool { + use BinaryOpKind::*; + matches!(self.kind, ShiftRight | ShiftLeft) + } } #[derive(Debug, Clone)] From dcb92e85c16030f0fafbacc421e981730096a1c1 Mon Sep 17 00:00:00 2001 From: guipublic Date: Mon, 31 Jul 2023 15:56:12 +0000 Subject: [PATCH 5/8] Check for signeness also during the delayed checks --- crates/noirc_frontend/src/hir/type_check/expr.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 92e3e619026..ec93400d834 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -894,6 +894,12 @@ impl<'interner> TypeChecker<'interner> { Err(TypeCheckError::InvalidBitwiseOperationOnField { span }) } else if other.is_bindable() { Err(TypeCheckError::AmbiguousBitWidth { span }) + } else if other.is_signed() { + Err(TypeCheckError::TypeCannotBeUsed { + typ: other, + place: "bit shift", + span, + }) } else { Ok(()) } From 12d795a033da98fbf0e2c62a18e60f80284de2bd Mon Sep 17 00:00:00 2001 From: guipublic Date: Mon, 31 Jul 2023 16:33:01 +0000 Subject: [PATCH 6/8] Add missing method --- crates/noirc_frontend/src/hir_def/types.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/noirc_frontend/src/hir_def/types.rs b/crates/noirc_frontend/src/hir_def/types.rs index 6e1113345a8..dcbc7521320 100644 --- a/crates/noirc_frontend/src/hir_def/types.rs +++ b/crates/noirc_frontend/src/hir_def/types.rs @@ -589,6 +589,10 @@ impl Type { matches!(self.follow_bindings(), Type::FieldElement(_)) } + pub fn is_signed(&self) -> bool { + matches!(self.follow_bindings(), Type::Integer(_, Signedness::Signed, _)) + } + fn contains_numeric_typevar(&self, target_id: TypeVariableId) -> bool { // True if the given type is a NamedGeneric with the target_id let named_generic_id_matches_target = |typ: &Type| { From dfdf4900485e1391d0e33688e4b641c1e65b30c4 Mon Sep 17 00:00:00 2001 From: guipublic Date: Tue, 1 Aug 2023 08:14:46 +0000 Subject: [PATCH 7/8] Code review --- crates/noirc_frontend/src/ast/expression.rs | 4 ++++ crates/noirc_frontend/src/hir/type_check/expr.rs | 4 ++-- crates/noirc_frontend/src/hir_def/expr.rs | 3 +-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/crates/noirc_frontend/src/ast/expression.rs b/crates/noirc_frontend/src/ast/expression.rs index e36f5b5d260..cae8f7f28d5 100644 --- a/crates/noirc_frontend/src/ast/expression.rs +++ b/crates/noirc_frontend/src/ast/expression.rs @@ -264,6 +264,10 @@ impl BinaryOpKind { BinaryOpKind::Modulo => Token::Percent, } } + + pub fn is_bit_shift(&self) -> bool { + matches!(self, BinaryOpKind::ShiftRight | BinaryOpKind::ShiftLeft) + } } #[derive(PartialEq, PartialOrd, Eq, Ord, Hash, Debug, Copy, Clone)] diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index ec93400d834..41a109baa01 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -885,7 +885,7 @@ impl<'interner> TypeChecker<'interner> { if op.is_bitwise() && (other.is_bindable() || other.is_field()) { let other = other.follow_bindings(); - + let kind = op.kind; // This will be an error if these types later resolve to a Field, or stay // polymorphic as the bit size will be unknown. Delay this error until the function // finishes resolving so we can still allow cases like `let x: u8 = 1 << 2;`. @@ -894,7 +894,7 @@ impl<'interner> TypeChecker<'interner> { Err(TypeCheckError::InvalidBitwiseOperationOnField { span }) } else if other.is_bindable() { Err(TypeCheckError::AmbiguousBitWidth { span }) - } else if other.is_signed() { + } else if kind.is_bit_shift() && other.is_signed() { Err(TypeCheckError::TypeCannotBeUsed { typ: other, place: "bit shift", diff --git a/crates/noirc_frontend/src/hir_def/expr.rs b/crates/noirc_frontend/src/hir_def/expr.rs index 5183cd44d9f..e2a61a8a222 100644 --- a/crates/noirc_frontend/src/hir_def/expr.rs +++ b/crates/noirc_frontend/src/hir_def/expr.rs @@ -74,8 +74,7 @@ impl HirBinaryOp { } pub fn is_bit_shift(&self) -> bool { - use BinaryOpKind::*; - matches!(self.kind, ShiftRight | ShiftLeft) + self.kind.is_bit_shift() } } From 1f108194cc0503e94c8756a55c81dcaabafee707 Mon Sep 17 00:00:00 2001 From: guipublic Date: Wed, 2 Aug 2023 08:50:57 +0000 Subject: [PATCH 8/8] Code review --- crates/noirc_frontend/src/hir/type_check/expr.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/noirc_frontend/src/hir/type_check/expr.rs b/crates/noirc_frontend/src/hir/type_check/expr.rs index 2692b2e4f83..24ac5f3443e 100644 --- a/crates/noirc_frontend/src/hir/type_check/expr.rs +++ b/crates/noirc_frontend/src/hir/type_check/expr.rs @@ -1008,7 +1008,7 @@ impl<'interner> TypeChecker<'interner> { }); } if op.is_bit_shift() - && (*sign_x != Signedness::Unsigned || *sign_y != Signedness::Unsigned) + && (*sign_x == Signedness::Signed || *sign_y == Signedness::Signed) { Err(TypeCheckError::InvalidInfixOp { kind: "Signed integer", span }) } else {