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

Unsigned integer division doesn't work with --experimental-ssa #1923

Closed
shuklaayush opened this issue Jul 13, 2023 · 1 comment · Fixed by #1928
Closed

Unsigned integer division doesn't work with --experimental-ssa #1923

shuklaayush opened this issue Jul 13, 2023 · 1 comment · Fixed by #1928
Labels
bug Something isn't working

Comments

@shuklaayush
Copy link
Contributor

Aim

use dep::std::println;

#[test]
fn test_div() {
    let a: u16 = 5;
    let b: u16 = 2;

    let c = a / b;

    // 0x183227397098d014dc2822db40c0ac2e9419f4243cdcb848a1f0fac9f8000003
    println(c);
    // This fails
    assert(c == 2);
}

Expected Behavior

The test should pass

Bug

Dividing unsigned ints with --experimental-ssa seems to calculate multiplication with the field inverse instead of standard integer division

To Reproduce

  1. Copy the above code
  2. Run the test
nargo test --show-output --experimental-ssa

Installation Method

Compiled from source

Nargo Version

nargo 0.8.0 (git version hash: 4969da7, is dirty: false)

Additional Context

No response

Would you like to submit a PR for this Issue?

Maybe

Support Needs

No response

@shuklaayush shuklaayush added the bug Something isn't working label Jul 13, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jul 13, 2023
@kevaundray
Copy link
Contributor

Thanks for opening this issue and bring it up on the call!

The bug is in this function:

When we have instructions with constant values, they get simplified on the fly. The problem is that our simplification pass is not taking into consideration the type. See division where it is doing lhs / rhs where these are FieldElements.

This method should be modified to check if we want to do field arithmetic or integer arithmetic and evaluate the constant expression accordingly.

Note: that this will only fail for constants. Non-constants won't be simplified and we do the division via acir_gen.

@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants