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

The compiler issues field comparisons for loops at the SSA level #1979

Closed
sirasistant opened this issue Jul 19, 2023 · 3 comments · Fixed by #2689
Closed

The compiler issues field comparisons for loops at the SSA level #1979

sirasistant opened this issue Jul 19, 2023 · 3 comments · Fixed by #2689
Assignees
Labels
bug Something isn't working discussion

Comments

@sirasistant
Copy link
Contributor

Aim

Consider the following code:

fn main() -> pub Field {
    let mut x = 0;
    for i in 0..10 {
        x += i;
    }
    x
}

Expected Behavior

The compiler is generating a comparison of i with the range 0..10 as can be seen in the following SSA:

Initial SSA:
acir fn main f0 {
  b0():
    v1 = allocate
    store Field 0 at v1
    jmp b1(Field 0)
  b1(v2: Field):
    v4 = lt v2, Field 10
    jmpif v4 then: b2, else: b3
  b2():
    v5 = load v1
    v6 = add v5, v2
    store v6 at v1
    v8 = add v2, Field 1
    jmp b1(v8)
  b3():
    v9 = load v1
    return v9
}

Which is a comparison with field operands, but the compiler does not allow the user to perform a similar kind of comparison themselves:

error: Comparisons are invalid on Field types. Try casting the operands to a sized integer type first
  ┌─ src/main.nr:5:12
  │
5 │         if i > 1 {
  │            -----

Bug

Since loops are unrolled for ACIR, this field comparison never reaches ACIR gen and is simplified away before. But for brillig, loops are runtime and the field comparison reaches brillig gen. This makes brillig having to handle Field comparisons generated by the compiler itself via casting to integer types.

To Reproduce

Installation Method

None

Nargo Version

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

@sirasistant sirasistant added the bug Something isn't working label Jul 19, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jul 19, 2023
@sirasistant sirasistant changed the title The compiler issues field comparisons for loops The compiler issues field comparisons for loops at the SSA level Jul 19, 2023
@jfecher
Copy link
Contributor

jfecher commented Jul 19, 2023

Note for when this is fixed, we can remove handling of this in brillig. See #1973 (comment)

@kevaundray
Copy link
Contributor

For reference: This is fixed if:

  • Cast x to an integer
  • Allow field comparisons to extend to > and <
  • Use a default type which is not a Field

@kevaundray
Copy link
Contributor

Have opened #2008 which feeds into this as just allowing Field to have < and > would be a possible fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working discussion
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants