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

Reintroduce shifts in SSA #4276

Closed
sirasistant opened this issue Feb 6, 2024 · 0 comments · Fixed by #4301
Closed

Reintroduce shifts in SSA #4276

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

Comments

@sirasistant
Copy link
Contributor

sirasistant commented Feb 6, 2024

Problem

SSA no longer has shifts, so any shift operation is polyfilled at the SSA level. However, brillig does support shifts, and the polyfill generates a huge amount of operations. This program:

unconstrained fn main(a: u64, b: u64) -> pub u64 {
    a >> b
}

Generates this SSA:

After Dead Instruction Elimination:
brillig fn main f0 {
  b0(v0: u32, v1: u32):
    v1189, v1190 = call to_le_bits(v1, Field 2⁵)
    v1191 = array_get v1190, index Field 31
    v1192 = not v1191
    v1193 = cast v1191 as Field
    v1194 = cast v1192 as Field
    v1195 = mul Field 2, v1193
    v1196 = add v1195, v1194
    v1197 = mul v1196, v1196
    v1198 = mul v1197, Field 2
    ... Repeated 30 more times
    v1469 = array_get v1190, index Field 0
    v1470 = not v1469
    v1471 = cast v1469 as Field
    v1472 = cast v1470 as Field
    v1473 = mul v1468, v1471
    v1474 = mul v1467, v1472
    v1475 = add v1473, v1474
    v1476 = div v0, v1475
    return v1476
}

This generates huge brillig programs for shifts that are natively supported by brillig.

Happy Case

We could reintroduce shifts in SSA, and polyfill them for ACIR functions in a step after inlining. After inlining all functions are colored (they'll only target their runtime) so we can convert any SSA level shift to the ACIR-compatible instructions and leave functions that target brillig unchanged.

Alternatives Considered

We could also implement shifts in acir_gen but that would miss on SSA-level optimizations.

Additional Context

No response

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 8, 2024
# Description

## Problem\*

Resolves #4276 

## Summary\*

Quick and dirty implementation of replacing bitshifts in SSA once
function inlining has occurred. Not a fan of how we are juggling the
instructions but will need to think about how to do this cleanly.

cc @sirasistant 

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