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

DIE pass visits blocks in wrong order and can remove still-used instructions #6465

Open
jfecher opened this issue Nov 6, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@jfecher
Copy link
Contributor

jfecher commented Nov 6, 2024

Aim

Performing DIE on the following brillig code (our loop test):

brillig(inline) fn main f0 {
  b0(v0: u32):
    v35 = allocate
    store u32 0 at v35
    jmp b1(u32 0)
  b1(v3: u32):
    v36 = lt v3, u32 4
    jmpif v36 then: b2, else: b3
  b2():
    v46 = load v35
    v47 = add v46, v3
    store v47 at v35
    v48 = add v3, u32 1
    jmp b1(v48)
  b3():
    v37 = load v35
    v38 = eq v37, v0
    constrain v37 == v0
    v39 = allocate
    store u32 0 at v39
    jmp b4(u32 0)
  b4(v13: u32):
    v40 = lt v13, u32 4
    jmpif v40 then: b5, else: b6
  b5():
    v43 = load v39
    v44 = add v43, v13
    store v44 at v39
    v45 = add v13, u32 1
    jmp b4(v45)
  b6():
    v41 = load v39
    v42 = eq v41, v0
    constrain v41 == v0
    return 
}
b0 -> b1 <-> b2
          -> b3 -> b4 <-> b5
                       -> b6

Expected Behavior

The visit order of the blocks should be:
b6, b4, b5, b3, b1, b2, b0
or
b6, b5, b4, b3, b2, b1, b0

Bug

The visit order of the blocks is actually:
b2, b5, b6, b4, b3, b1, b0

To Reproduce

Workaround

None

Workaround Description

No response

Additional Context

No response

Project Impact

None

Blocker Context

No response

Nargo Version

No response

NoirJS Version

No response

Proving Backend Tooling & Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

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

jfecher commented Nov 6, 2024

As far as I can tell the only reason this doesn't cause errors already is because we don't remove stores from DIE and other instructions typically can't be accessed after a loop in that way without introducing a new ValueId.

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
Status: 📋 Backlog
Development

No branches or pull requests

1 participant