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

Implementation of unsigned overflow checks is ACIR specific #4456

Closed
sirasistant opened this issue Feb 29, 2024 · 0 comments · Fixed by #4832
Closed

Implementation of unsigned overflow checks is ACIR specific #4456

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

Comments

@sirasistant
Copy link
Contributor

sirasistant commented Feb 29, 2024

Problem

Overflow checks are added on ssa gen. But the overflow checks for signed integers only make sense for the specifics of ACIR, and don't make much sense in SSA. Take this example:

fn main(a: u8, b: u8) -> pub u8 {
    a * b
}

This generates the following SSA:

acir fn main f0 {
  b0(v0: u8, v1: u8):
    v2 = mul v0, v1
    range_check v2 to 8 bits
    return v2
}

v2 has type of u8. In SSA land, range checking a u8 to just have 8 bits should be a no-op. It only works because it knows that in ACIR additions of u8s are implemented as field additions so adding up two u8s could generate a result larger than a u8 in case of overflow.

This makes the current SSA-gen overflow checks to be no-ops in brillig, and they add overhead because we'd codegen a redundant range check (I'll open a PR to try to catch redundant range checks from generating any bytecode)

Note: this is only for unsigned. Signed overflow checks appear to apply correctly to brillig

Happy Case

We figure out a way of having efficient overflow checks with a clean abstraction. If we did at the SSA gen level the overflow checking in the style that brillig gen does it it'd generate more constraints for acir functions.

Two options that I can think of:

  • Do overflow checking in acir_gen/brillig_gen
  • Do overflow checking in a specific SSA pass that is aware of the target runtime of each function and codegens an efficient version for each one

Project Impact

None

Impact Context

No response

Workaround

None

Workaround Description

A PR was added to add the unsigned overflow checks into brillig gen directly #4445.

Additional Context

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@sirasistant sirasistant added the enhancement New feature or request label Feb 29, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Feb 29, 2024
github-merge-queue bot pushed a commit that referenced this issue Mar 1, 2024
# Description

## Problem\*

Avoid codegening overhead for no-op range checks generated by
#4456

## 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\*

- [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.
TomAFrench added a commit that referenced this issue May 8, 2024
# Description

## Problem\*

Resolves #4456

## Summary\*

Overflow checks for unsigned integers are done in acir-gen, when
converting arithmetic operations into field arithmetic. That way the
semantic of the ssa unsigned operations is preserved.

## Additional Context

As indicated in the issue, I did not touch signed integers, but it could
be cleaner to convert them into unsigned operations (and have the signed
overflow checks done with additional ssa instructions, like we do
currently)

## 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.
- [X] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Tom French <tom@tomfren.ch>
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir May 8, 2024
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
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants