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.

Sign up for GitHub

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

Jump to bottom

bug(noir): Noir struggles to remove dead code created by context objects when a loop is injected #4979

Closed
Maddiaa0 opened this issue Mar 6, 2024 · 1 comment
Closed

bug(noir): Noir struggles to remove dead code created by context objects when a loop is injected #4979

Maddiaa0 opened this issue Mar 6, 2024 · 1 comment
Labels
C-avm Component: AVM related tickets (aka public VM) C-noir Component: Noir/Nargo T-bug Type: Bug. Something is broken.

Comments

@Maddiaa0
Copy link
Member

Maddiaa0 commented Mar 6, 2024 •
edited
Loading

repro: md/repro-brillig-explosion

When refactoring the public_state.write method to write one field at a time, (rather than writing an entire array of fields at once) I have come across an interesting loop unrolling issue in brillig gen. I've spent some time diving into the brillig code to work out where the issue is.

I changed
let fields = T::serialize(value);
storage_write(self.storage_slot, fields);

to

let fields = T::serialize(value);
for i in 0..fields.len() {
    storage_write(self.storage_slot + i, fields[i]);
}

And saw the number of brillig opcodes jump from ~10 to 2000.
I originally thought it was some generic issue, but it turns out that if you just replace the code with

for i in 0..1 {
    println(i);
}

Where the loop should only run once, then the same blow up to 2000 opcodes exists.
[https://github.com/AztecProtocol/aztec-packages/blob/58da8815a68642ea78980de2a6d0f[…]df7/noir-projects/aztec-nr/aztec/src/state_vars/public_state.nr
](

aztec-packages/noir-projects/aztec-nr/aztec/src/state_vars/public_state.nr

Line 43 in 58da881

for i in 0..1 {
)

I have done it on this branch, just run

directory: /noir-projects/noir-contracts
command: nargo compile --package avm_test_contract --silence-warnings

and see the brillig output (show_brillig is hardcoded to true)
To see what the output previously was you can comment the loop, and uncomment storage_writes to see the number of contraints go back to normal

https://github.com/AztecProtocol/aztec-packages/blob/58da8815a68642ea78980de2a6d0f1bbbed88df7/noir-projects/aztec-nr/aztec/src/state_vars/public_state.nr
for i in 0..1 {

From the debugging that I have done, I think the inclusion of the loop breaks noir's dead code elimination somewhat. Where the Option::none() in the context object are not removed.

aztec-packages/noir-projects/aztec-nr/aztec/src/context.nr

Line 4 in d3fc4f1

mod private_context;

@github-project-automation github-project-automation bot moved this to Todo in A3 Mar 6, 2024
@Maddiaa0 Maddiaa0 mentioned this issue Mar 6, 2024
Merged
Maddiaa0 added a commit that referenced this issue Mar 6, 2024
@Maddiaa0 @sirasistant
feat(avm): storage (#4673)
bfdbf2e 
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
#4814

---------

Co-authored-by: sirasistant <sirasistant@gmail.com>
AztecBot added a commit to noir-lang/noir that referenced this issue Mar 6, 2024
@AztecBot @sirasistant
feat(avm): storage (AztecProtocol/aztec-packages#4673)
50e001a 
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.

- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <sirasistant@gmail.com>
@Maddiaa0 Maddiaa0 mentioned this issue Mar 7, 2024
Merged
Maddiaa0 added a commit that referenced this issue Mar 7, 2024
@Maddiaa0 @sirasistant
fix: storage v2 (#5027)
fe3190e 
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
#4814

---------

Co-authored-by: sirasistant <sirasistant@gmail.com>
AztecBot added a commit to noir-lang/noir that referenced this issue Mar 7, 2024
@AztecBot @sirasistant
fix: storage v2 (AztecProtocol/aztec-packages#5027)
699f43d 
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.

- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <sirasistant@gmail.com>
AztecBot added a commit to noir-lang/noir that referenced this issue Mar 7, 2024
@AztecBot @sirasistant
fix: storage v2 (AztecProtocol/aztec-packages#5027)
0bba5e8 
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.

- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <sirasistant@gmail.com>
AztecBot pushed a commit to AztecProtocol/barretenberg that referenced this issue Mar 8, 2024
@Maddiaa0 @sirasistant @AztecBot
fix: storage v2 (#5027)
25b4d7a 
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <sirasistant@gmail.com>
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue Mar 19, 2024
@Maddiaa0 @sirasistant @AztecBot
feat(avm): storage (#4673)
693c4b4 
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <sirasistant@gmail.com>
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue Mar 19, 2024
@Maddiaa0 @sirasistant @AztecBot
fix: storage v2 (#5027)
58d77a6 
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <sirasistant@gmail.com>
superstar0402 added a commit to superstar0402/aztec-nr that referenced this issue Aug 16, 2024
@superstar0402 @sirasistant
feat(avm): storage (#4673)
de11d65 
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <sirasistant@gmail.com>
superstar0402 added a commit to superstar0402/aztec-nr that referenced this issue Aug 16, 2024
@superstar0402 @sirasistant
fix: storage v2 (#5027)
b485cf5 
## Overview
Works around brillig blowup issue by altering the read and write opcodes
to take in arrays of data. This is potentially just a short term fix.

- Reading and writing to storage now take in arrays, code will not
compile without this change, due to an ssa issue ->[ ISSUE
](AztecProtocol/aztec-packages#4979)

- Tag checks on memory now just make sure the tag is LESS than uint64,
rather than asserting that the memory tag is uint32, this should be
fine.


- We had to blow up the memory space of the avm to u64 as the entire
noir compiler works with u64s. Arrays will not work unless we either
    - Make the avm 64 bit addressable ( probably fine )
- Make noir 32 bit addressable ( requires alot of buy in )
AztecProtocol/aztec-packages#4814

---------

Co-authored-by: sirasistant <sirasistant@gmail.com>
@Maddiaa0
Copy link
Member Author

Maddiaa0 commented Oct 2, 2024

The contents of this issue are captured by noir-lang/noir#4535

@Maddiaa0 Maddiaa0 closed this as completed Oct 2, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in A3 Oct 2, 2024
@Maddiaa0 Maddiaa0 added T-bug Type: Bug. Something is broken. C-noir Component: Noir/Nargo C-avm Component: AVM related tickets (aka public VM) labels Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-avm Component: AVM related tickets (aka public VM) C-noir Component: Noir/Nargo T-bug Type: Bug. Something is broken.
Projects
A3
Archived in project
Milestone
No milestone
Development

No branches or pull requests
1 participant
@Maddiaa0