From 0cf2e2a1b8d247bed03ba5b7b1be5cd30f0d51b2 Mon Sep 17 00:00:00 2001 From: jfecher Date: Wed, 24 Apr 2024 11:55:27 -0500 Subject: [PATCH] fix: Nested array equality (#4903) # Description ## Problem\* Resolves #4383 (both examples in the issue) ## Summary\* We were relying on the built in array equality for arrays in SSA previously, but this did not recurse on the array element in the case of nested arrays. I've just removed this since it is no longer needed. We now use the impl Eq for arrays in the stdlib instead since the built in version provided no speedup anyway. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Tom French --- .../src/ssa/ssa_gen/context.rs | 108 +----------------- .../noirc_frontend/src/hir/type_check/expr.rs | 30 ----- .../regression_4383/Nargo.toml | 7 ++ .../regression_4383/src/main.nr | 3 + 4 files changed, 15 insertions(+), 133 deletions(-) create mode 100644 test_programs/execution_success/regression_4383/Nargo.toml create mode 100644 test_programs/execution_success/regression_4383/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index e591a3d478c..276a8247cea 100644 --- a/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -566,22 +566,12 @@ impl<'a> FunctionContext<'a> { mut rhs: ValueId, location: Location, ) -> Values { - let result_type = self.builder.type_of_value(lhs); - let mut result = match operator { - BinaryOpKind::Equal | BinaryOpKind::NotEqual - if matches!(result_type, 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); - } + 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) - } - }; + let mut result = self.builder.set_location(location).insert_binary(lhs, op, rhs); // Check for integer overflow if matches!( @@ -600,94 +590,6 @@ impl<'a> FunctionContext<'a> { result.into() } - /// The frontend claims to support equality (==) on arrays, so we must support it in SSA here. - /// The actual BinaryOp::Eq in SSA is meant only for primitive numeric types so we encode an - /// entire equality loop on each array element. The generated IR is as follows: - /// - /// ... - /// result_alloc = allocate - /// store u1 1 in result_alloc - /// jmp loop_start(0) - /// loop_start(i: Field): - /// v0 = lt i, array_len - /// jmpif v0, then: loop_body, else: loop_end - /// loop_body(): - /// v1 = array_get lhs, index i - /// v2 = array_get rhs, index i - /// v3 = eq v1, v2 - /// v4 = load result_alloc - /// v5 = and v4, v3 - /// store v5 in result_alloc - /// v6 = add i, Field 1 - /// jmp loop_start(v6) - /// loop_end(): - /// result = load result_alloc - fn insert_array_equality( - &mut self, - lhs: ValueId, - operator: BinaryOpKind, - rhs: ValueId, - location: Location, - ) -> Values { - let lhs_type = self.builder.type_of_value(lhs); - let rhs_type = self.builder.type_of_value(rhs); - - let (array_length, element_type) = match (lhs_type, rhs_type) { - ( - Type::Array(lhs_composite_type, lhs_length), - Type::Array(rhs_composite_type, rhs_length), - ) => { - assert!( - lhs_composite_type.len() == 1 && rhs_composite_type.len() == 1, - "== is unimplemented for arrays of structs" - ); - assert_eq!(lhs_composite_type[0], rhs_composite_type[0]); - assert_eq!(lhs_length, rhs_length, "Expected two arrays of equal length"); - (lhs_length, lhs_composite_type[0].clone()) - } - _ => unreachable!("Expected two array values"), - }; - - let loop_start = self.builder.insert_block(); - let loop_body = self.builder.insert_block(); - let loop_end = self.builder.insert_block(); - - // pre-loop - let result_alloc = self.builder.set_location(location).insert_allocate(Type::bool()); - let true_value = self.builder.numeric_constant(1u128, Type::bool()); - self.builder.insert_store(result_alloc, true_value); - let zero = self.builder.length_constant(0u128); - self.builder.terminate_with_jmp(loop_start, vec![zero]); - - // loop_start - self.builder.switch_to_block(loop_start); - let i = self.builder.add_block_parameter(loop_start, Type::length_type()); - let array_length = self.builder.length_constant(array_length as u128); - let v0 = self.builder.insert_binary(i, BinaryOp::Lt, array_length); - self.builder.terminate_with_jmpif(v0, loop_body, loop_end); - - // loop body - self.builder.switch_to_block(loop_body); - let v1 = self.builder.insert_array_get(lhs, i, element_type.clone()); - let v2 = self.builder.insert_array_get(rhs, i, element_type); - let v3 = self.builder.insert_binary(v1, BinaryOp::Eq, v2); - let v4 = self.builder.insert_load(result_alloc, Type::bool()); - let v5 = self.builder.insert_binary(v4, BinaryOp::And, v3); - self.builder.insert_store(result_alloc, v5); - let one = self.builder.length_constant(1u128); - let v6 = self.builder.insert_binary(i, BinaryOp::Add, one); - self.builder.terminate_with_jmp(loop_start, vec![v6]); - - // loop end - self.builder.switch_to_block(loop_end); - let mut result = self.builder.insert_load(result_alloc, Type::bool()); - - if operator_requires_not(operator) { - result = self.builder.insert_not(result); - } - result.into() - } - /// Inserts a call instruction at the end of the current block and returns the results /// of the call. /// diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs index 0bc7673e105..7b30777c3ba 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -890,36 +890,6 @@ impl<'interner> TypeChecker<'interner> { // <= and friends are technically valid for booleans, just not very useful (Bool, Bool) => Ok((Bool, false)), - // Special-case == and != for arrays - (Array(x_size, x_type), Array(y_size, y_type)) - if matches!(op.kind, BinaryOpKind::Equal | BinaryOpKind::NotEqual) => - { - self.unify(x_size, y_size, || TypeCheckError::TypeMismatchWithSource { - expected: lhs_type.clone(), - actual: rhs_type.clone(), - source: Source::ArrayLen, - span: op.location.span, - }); - - let (_, use_impl) = self.comparator_operand_type_rules(x_type, y_type, op, span)?; - - // If the size is not constant, we must fall back to a user-provided impl for - // equality on slices. - let size = x_size.follow_bindings(); - let use_impl = use_impl || size.evaluate_to_u64().is_none(); - Ok((Bool, use_impl)) - } - - (String(x_size), String(y_size)) => { - self.unify(x_size, y_size, || TypeCheckError::TypeMismatchWithSource { - expected: *x_size.clone(), - actual: *y_size.clone(), - span: op.location.span, - source: Source::StringLen, - }); - - Ok((Bool, false)) - } (lhs, rhs) => { self.unify(lhs, rhs, || TypeCheckError::TypeMismatchWithSource { expected: lhs.clone(), diff --git a/test_programs/execution_success/regression_4383/Nargo.toml b/test_programs/execution_success/regression_4383/Nargo.toml new file mode 100644 index 00000000000..2ad8c208f24 --- /dev/null +++ b/test_programs/execution_success/regression_4383/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_4383" +type = "bin" +authors = [""] +compiler_version = ">=0.26.0" + +[dependencies] diff --git a/test_programs/execution_success/regression_4383/src/main.nr b/test_programs/execution_success/regression_4383/src/main.nr new file mode 100644 index 00000000000..fec63ea64a5 --- /dev/null +++ b/test_programs/execution_success/regression_4383/src/main.nr @@ -0,0 +1,3 @@ +fn main() { + assert([[1]] == [[1]]); +}