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_tail_to_public::tests::enqueued_public_calls_with_teardown_gas fails in master #6674

Closed
TomAFrench opened this issue Dec 2, 2024 · 2 comments · Fixed by #6687
Assignees
Labels
bug Something isn't working

Comments

@TomAFrench
Copy link
Member

This test is currently showing as failing in our external repo checks: https://github.com/noir-lang/noir/actions/runs/12123521659/job/33799364171?pr=6672#step:6:50

We should pull a reproduction case into this repo and fix it.

@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Dec 2, 2024
@TomAFrench TomAFrench added the bug Something isn't working label Dec 2, 2024
@asterite
Copy link
Collaborator

asterite commented Dec 2, 2024

So far what I found is that reverting #6088 makes the test pass.

I also found that this SSA:

v258 = load v252 -> [Field; 3]
inc_rc v258
v265 = load v252 -> [Field; 3]
inc_rc v265

becomes this with that optimization above:

v258 = load v252 -> [Field; 3]
inc_rc v258
inc_rc v258

That's because we can't find a store between the two loads to the same address, so they are unified. But this means we end up with two inc_rc for the same array, where previously we only had one. I don't know if that could lead to incorrectness or just to a performance regression (there was one in that PR, maybe it's related?)

@jfecher
Copy link
Contributor

jfecher commented Dec 2, 2024

But this means we end up with two inc_rc for the same array, where previously we only had one.

They're both loaded from the same address with no stores in between so it should be the same array loaded each time. So the first example should be equivalent to the second - not sure why this would cause issues.

github-merge-queue bot pushed a commit that referenced this issue Dec 6, 2024
Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com>
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Dec 6, 2024
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: ✅ Done
3 participants