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: Extra mem2reg pass causing failures for UHashMap #5897

Closed
vezenovm opened this issue Sep 3, 2024 · 1 comment
Closed

SSA: Extra mem2reg pass causing failures for UHashMap #5897

vezenovm opened this issue Sep 3, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@vezenovm
Copy link
Contributor

vezenovm commented Sep 3, 2024

Aim

While working on #5865 our brillig_loop_size_regression program is left with a bunch of extra stores and allocates following DIE which removes any loads that reference those stores. As a quick solution to test getting to a minimal bytecode I placed an extra mem2reg and DIE pass following the previously final DIE pass. The two extra passes are not an ideal solution for #5865 due to efficiency reasons, but it is still worrisome that uhashmap would fail given an extra mem2reg and DIE pass so I am taking note of that in this issue.

Expected Behavior

I should be able to insert mem2reg anywhere in the compilation flow and not cause test failures.

Bug

Running mem2reg and DIE after the already existing DIE causes uhashmap to fail with a type mismatch:

error: Assertion failed: 'Bit size for lhs 0 does not match op bit size 32'
    ┌─ std/collections/umap.nr:358:12
    │
358 │   if self.len() + 1 >= self.capacity() / 2 {

To Reproduce

  1. You can reference this commit a595349 for more context about failures.
  2. The easiest way to reproduce is to add an extra mem2reg and die pass after our existing die pass.
  3. For both strategies recompile and execute execution_success/uhashmap

Workaround

Yes

Workaround Description

Do not run mem2reg after DIE

Additional Context

No response

Project Impact

Blocker

Blocker Context

Not a current blocker, but definitely has the potential to block in the future

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

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

Closing this as the cause this failure was due to inappropriately accounting for reference params #5865 (comment) and AztecProtocol/aztec-packages#8448 removed the need for a re-load and store when laying down inc/dec rc instruction

@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Sep 19, 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
Development

No branches or pull requests

1 participant