Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make types in SSA binary operations match #4275

Closed
6 tasks done
sirasistant opened this issue Feb 6, 2024 · 0 comments · Fixed by #4391
Closed
6 tasks done

Make types in SSA binary operations match #4275

sirasistant opened this issue Feb 6, 2024 · 0 comments · Fixed by #4391
Assignees
Labels
enhancement New feature or request

Comments

@sirasistant
Copy link
Contributor

sirasistant commented Feb 6, 2024

Problem

Upcoming changes in brillig will require that the types of both operands in binary operations match. Even though in noir we enforce matching types, we don't in SSA, and the SSA codegen sometimes issues binary operations where the types of LHS and RHS don't match.

Happy Case

This assertion shouldn't fail in tests:

    pub(crate) fn insert_binary(
        &mut self,
        lhs: ValueId,
        operator: BinaryOp,
        rhs: ValueId,
    ) -> ValueId {
        assert_eq!(
            self.type_of_value(lhs),
            self.type_of_value(rhs),
            "ICE - Binary operands must have the same type"
        );

Alternatives Considered

We could upcast in the codegen as we do now. But brillig for the AVM won't support euclidean divisions of field types, so this would still be an issue. Also, it makes sense for SSA to respect this binary operation rule as noir does, and it would simplify codegens after SSA.

Additional Context

Current cases where this check is blowing up

Initial tests failing with this check:

failures:
    tests::compile_failure_radix_non_constant_length
    tests::compile_failure_slice_access_failure
    tests::compile_failure_slice_insert_failure
    tests::compile_failure_slice_remove_failure
    tests::compile_success_empty_brillig_integer_binary_operations
    tests::compile_success_empty_closure_explicit_types
    tests::compile_success_empty_conditional_regression_547
    tests::compile_success_empty_conditional_regression_to_bits
    tests::compile_success_empty_ec_baby_jubjub
    tests::compile_success_empty_field_comparisons
    tests::compile_success_empty_regression_3635
    tests::compile_success_empty_str_as_bytes
    tests::compile_success_empty_to_bits
    tests::compile_success_empty_vectors
    tests::execution_success_6
    tests::execution_success_6_array
    tests::execution_success_7_function
    tests::execution_success_array_dynamic
    tests::execution_success_bit_shifts_comptime
    tests::execution_success_bit_shifts_runtime
    tests::execution_success_brillig_cow
    tests::execution_success_brillig_cow_assign
    tests::execution_success_brillig_hash_to_field
    tests::execution_success_brillig_loop
    tests::execution_success_brillig_oracle
    tests::execution_success_brillig_pedersen
    tests::execution_success_brillig_schnorr
    tests::execution_success_brillig_slices
    tests::execution_success_brillig_to_be_bytes
    tests::execution_success_brillig_to_bits
    tests::execution_success_brillig_to_bytes_integration
    tests::execution_success_brillig_to_le_bytes
    tests::execution_success_conditional_1
    tests::execution_success_databus
    tests::execution_success_eddsa
    tests::execution_success_hash_to_field
    tests::execution_success_integer_array_indexing
    tests::execution_success_merkle_insert
    tests::execution_success_modulus
    tests::execution_success_nested_array_in_slice
    tests::execution_success_operator_overloading
    tests::execution_success_pedersen_check
    tests::execution_success_poseidon_bn254_hash
    tests::execution_success_poseidonsponge_x5_254
    tests::execution_success_references
    tests::execution_success_regression
    tests::execution_success_schnorr
    tests::execution_success_sha2_byte
    tests::execution_success_simple_radix
    tests::execution_success_simple_shield
    tests::execution_success_simple_shift_left_right
    tests::execution_success_slice_dynamic_index
    tests::execution_success_slices
    tests::execution_success_to_be_bytes
    tests::execution_success_to_bytes_consistent
    tests::execution_success_to_bytes_integration
    tests::execution_success_to_le_bytes
    tests::execution_success_tuple_inputs
    tests::execution_success_u128
    tests::noir_test_success_field_comparisons

Current tests failing with this check:


Would you like to submit a PR for this Issue?

No

Support Needs

No response

@sirasistant sirasistant added the enhancement New feature or request label Feb 6, 2024
github-merge-queue bot pushed a commit that referenced this issue Feb 6, 2024
# Description

## Problem\*

Partial work towards #4275

## Summary\*



## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [ ] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
github-merge-queue bot pushed a commit that referenced this issue Feb 7, 2024
# Description

## Problem\*

Partial work towards #4275

## Summary\*

We were mixing Field and u64 types when accessing arrays. Since we index
arrays by u64s, always cast array/slice addressing values to u64.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
github-merge-queue bot pushed a commit that referenced this issue Feb 7, 2024
# Description

## Problem\*

Partial work towards #4275

## Summary\*

Also use the array index type (u64) for assign_lvalue_index.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
github-merge-queue bot pushed a commit that referenced this issue Feb 8, 2024
# Description

## Problem\*

Partial work towards #4275

## Summary\*

Previously, SSA was issuing (lhs_type, Field) DIV for right shifts.
However this is wrong if we support shifts by the bit size (`u1 >> 1`)

This is interpreted in acir_gen as an euclidean division with bit_size =
lhs_type.bit_size(). However, u1 >> 1 worked because of a + 1 that is in
euclidean division
https://github.com/noir-lang/noir/blob/0e073037b00212fd17fc8ca9c531b567614eb4c5/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs#L703
(and it failed in unconstrained functions since they don't codegen that
extraneous + 1)

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: TomAFrench <tom@tomfren.ch>
github-merge-queue bot pushed a commit that referenced this issue Feb 13, 2024
# Description

## Problem\*

Partial work towards #4275

## Summary\*



## Additional Context

Tested in aztec-packages here
AztecProtocol/aztec-packages#4556

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
github-merge-queue bot pushed a commit that referenced this issue Feb 16, 2024
# Description

## Problem\*

Resolves #4275

## Summary\*

Adds a check in insert_binary to make sure we don't start codegening
non-matching binary ops again.


## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** Documentation to be submitted in a separate
PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant