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

private-kernel-lib tests are failing on Noir master #6665

Closed
TomAFrench opened this issue Nov 30, 2024 · 3 comments · Fixed by #6672
Closed

private-kernel-lib tests are failing on Noir master #6665

TomAFrench opened this issue Nov 30, 2024 · 3 comments · Fixed by #6672
Assignees

Comments

@TomAFrench
Copy link
Member

See CI run which runs nargo test on the protocol circuit tests: https://github.com/noir-lang/noir/actions/runs/12091844455/job/33720813101?pr=6660

These are all tests which don't make use of custom foreign calls so they should be relatively simple to pull out into this repository, minimise and create a regression test.

@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Nov 30, 2024
@asterite asterite self-assigned this Dec 2, 2024
@asterite
Copy link
Collaborator

asterite commented Dec 2, 2024

Debugging this a bit, it seems the issue is with instruction hoisting in constant folding. There's code like this:

        if num_note_hashes != 0 {
            let x = num_note_hashes - 1;

and we get attempt to subtract with overflow likely because that subtraction is done even if num_note_hashes is zero. I still need to figure out exactly what's the issue, but disabling that hoisting fixes it.

@asterite
Copy link
Collaborator

asterite commented Dec 2, 2024

@TomAFrench @jfecher We are hoisting any instruction that doesn't have side effects. But sub (and other binary math operations?) can lead to underflow/overflow so we can't always hoist it: if that sub can only be done after we check that a value is not zero, doing it before that will lead to that panic.

I guess we can consider Add, Sub, Mul and Div as having side effects? Alternatively we can drop the has_side_effects name and just use can_be_hoisted.

What do you think?

@TomAFrench
Copy link
Member Author

Answered in #6672 but yeah we should disable this optimization on any instructions which break it so that we can get tests passing (as it's a relatively recent optimization this won't cause a deal-breaking regression). We can then look at how to optimize more of these instructions in followups.

@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

2 participants