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

(SSA Refactor) Printer prints old ValueId after set_value is called #1451

Closed
Tracked by #1376
jfecher opened this issue May 30, 2023 · 2 comments · Fixed by #1535
Closed
Tracked by #1376

(SSA Refactor) Printer prints old ValueId after set_value is called #1451

jfecher opened this issue May 30, 2023 · 2 comments · Fixed by #1535
Labels
bug Something isn't working refactor ssa

Comments

@jfecher
Copy link
Contributor

jfecher commented May 30, 2023

Aim

Running the mem2reg pass on the code:

fn main f0 {
  b0(v0: u1, v1: u1):
    v2 = alloc 1 fields
    store Field 0 at v2
    v7 = load v2
    call println(Field 0, v7)
    store Field 1 at v2
    v40 = load v2
    call println(Field 1, v40)
    v42 = load v2
    store Field 2 at v2
    v43 = load v2
    call println(Field 2, v43)
    v44 = load v2
    call println(Field 4, v44)
    v45 = and v0, v1
    v47 = load v2
    store Field 5 at v2
    v48 = load v2
    call println(Field 5, v48)
    v49 = not v1
    v50 = and v0, v49
    v52 = load v2
    store Field 6 at v2
    v53 = load v2
    call println(Field 6, v53)
    v54 = mul v1, Field 5
    v55 = mul v49, v47
    v56 = add v54, v55
    store v56 at v2
    v57 = mul v1, v52
    v58 = mul v49, Field 6
    v59 = add v57, v58
    store v59 at v2
    v60 = load v2
    call println(Field 7, v60)
    v61 = not v0
    v63 = load v2
    store Field 3 at v2
    v64 = load v2
    call println(Field 3, v64)
    v65 = load v2
    call println(Field 8, v65)
    v66 = mul v0, v59
    v67 = mul v61, v42
    v68 = add v66, v67
    store v68 at v2
    v69 = mul v0, v63
    v70 = mul v61, Field 3
    v71 = add v69, v70
    store v71 at v2
    v72 = load v2
    call println(Field 9, v72)
    return 
}

Expected Behavior

The mem2reg pass should replace all known stores with the known values and leave stores that are unknown. Failing that, the pass should at least leave the program in a valid state.

Bug

The actual SSA returned by the mem2reg pass is:

fn main f0 {
  b0(v0: u1, v1: u1):
    v2 = alloc 1 fields
    store Field 0 at v2
    call println(Field 0, Field 0)
    store Field 1 at v2
    call println(Field 1, Field 1)
    store Field 2 at v2
    call println(Field 2, Field 2)
    call println(Field 4, Field 2)
    v45 = and v0, v1
    store Field 5 at v2
    call println(Field 5, Field 5)
    v49 = not v1
    v50 = and v0, v49
    store Field 6 at v2
    call println(Field 6, Field 6)
    v54 = mul v1, Field 5
    v55 = mul v49, Field 2
    v56 = add v54, v55
    store v56 at v2
    v57 = mul v1, Field 5
    v58 = mul v49, Field 6
    v59 = add v57, v58
    store v59 at v2
    call println(Field 7, v60)
    v61 = not v0
    store Field 3 at v2
    call println(Field 3, Field 3)
    call println(Field 8, Field 3)
    v66 = mul v0, v59
    v67 = mul v61, Field 1
    v68 = add v66, v67
    store v68 at v2
    v69 = mul v0, v63
    v70 = mul v61, Field 3
    v71 = add v69, v70
    store v71 at v2
    call println(Field 9, v72)
    return 
}

Note v60 which is referenecd by call println(Field 7, v60) but there is no corresponding definition for v60.

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

@jfecher jfecher added the bug Something isn't working label May 30, 2023
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir May 30, 2023
@jfecher
Copy link
Contributor Author

jfecher commented May 30, 2023

I think this is likely caused by set_value replacing instructions but the SSA printer prints out the old id which can be confusing since it is aliased to a new value.

@jfecher jfecher changed the title (SSA Refactor) Mem2Reg pass removes loads which are still referenced (SSA Refactor) Printer prints old ValueId after set_value is called May 30, 2023
@jfecher
Copy link
Contributor Author

jfecher commented May 30, 2023

I don't think this is fixable without redundantly storing the ValueId within the Value enum

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

Successfully merging a pull request may close this issue.

2 participants