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

fix: Fix remaining issues in Brillig's copy on write arrays #3593

Merged
merged 8 commits into from
Nov 28, 2023

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Nov 27, 2023

Description

Problem*

Fixes remaining todo and some panics in #3522

Summary*

I'm not sure if unpacking the entire slice recursively to complete the todo!() is even possible so I tried a different (hopefully sound) approach of incrementing the Rc for sub-arrays on each ArrayGet and when constructing the array. I've also disabled the IncrementRc instruction when not in Brillig code since it is unneeded.

Additional Context

I'm targetting @sirasistant's PR so that it is easier to see the changes I made to it. If all is well, we can merge this PR and then his afterward to get the feature in.

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@jfecher
Copy link
Contributor Author

jfecher commented Nov 27, 2023

Testing this on noir-rsa I get the following panic:

[rsa] Testing test_verify_sha256_pkcs1v15_512... The application panicked (crashed).
Message:  internal error: entered unreachable code: ICE: Expected vector, got BrilligArray(BrilligArray { pointer: RegisterIndex(45), size: 56, rc: RegisterIndex(46) })
Location: compiler/noirc_evaluator/src/brillig/brillig_ir/brillig_variable.rs:74

@jfecher
Copy link
Contributor Author

jfecher commented Nov 27, 2023

Interesting, the compile_success_empty_brillig_integer_binary_operations test is failing but only when running cargo t and not cargo t --release

---- tests::compile_success_empty_brillig_integer_binary_operations stdout ----
thread 'tests::compile_success_empty_brillig_integer_binary_operations' panicked at '`nargo info` failed with: The application panicked (crashed).
Message:  internal error: entered unreachable code: ICE: Expected vector, got BrilligArray(BrilligArray { pointer: RegisterIndex(6), size: 32, rc: RegisterIndex(7) })
Location: compiler/noirc_evaluator/src/brillig/brillig_ir/brillig_variable.rs:74

Edit: This was on linux. On mac it looks like it panics in both debug and release.

@jfecher jfecher requested a review from sirasistant November 27, 2023 19:23
Copy link
Contributor

@sirasistant sirasistant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me!

@jfecher jfecher merged commit 2030653 into arv/brillig_reference_counters Nov 28, 2023
33 checks passed
@jfecher jfecher deleted the jf/brillig-cow branch November 28, 2023 19:52
github-merge-queue bot pushed a commit that referenced this pull request Nov 28, 2023
* Add element type to Type::Reference

* Add inc_rc instruction

* Add inc_rc instructions to ssa programs

* Implement inc_rc in brillig

* Implement copy on write optimization in brillig

* Refactor sections

* Fix initial reference count

* Add reference count to extract_registers

* Remove dbg panic

* Cleanup branch_instruction

* Start refactor to store rc in the first element of the array

* Add reference type to tests

* revert: set brillig gen to master

* wip: refactor brillig variable vs registerormemory

* feat: cow optimization

* test: restore slice tests

* refactor: typed references

* test: working on fixing entry point tests

* test: fix entry point tests

* test: fix dfg test

* fix: remove safety check and address todos

* docs: variable naming and comments

* chore: formatting

* revert: cargo lock

* fix: Fix remaining issues in Brillig's copy on write arrays (#3593)

* Remove todo by going with new approach to Rc

* Very important commit: fix typo

* fix: make toBits accept arrays and add test

* fix: always codegen inc_rc

* IncrementRC has no side effects?

* Special case inc_rc instead

* IncRc is actually an even more special case of DIE. Also mark Value::Params as used

* Remove abbreviation

---------

Co-authored-by: sirasistant <sirasistant@gmail.com>

---------

Co-authored-by: Jake Fecher <jfecher11@gmail.com>
Co-authored-by: Jake Fecher <jake@aztecprotocol.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants