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(ssa): Tracking slices stores correctly for parent blocks and handling in ACIR #4221

Closed
wants to merge 8 commits into from

Conversation

vezenovm
Copy link
Contributor

@vezenovm vezenovm commented Feb 1, 2024

Description

Problem*

Resolves #4202

Summary*

There are two fixes done in this PR, one in the SSA and one in the ACIR:

SSA Update

When storing slice stores from the outer block we were only checking JmpIf terminators. The issue has provided an example SSA where we jump to a block which performs a jump if without the block with the jump if storing any values.

Here is the SSA for the basic failing program in the linked issue:

After Mem2Reg:
acir fn main f0 {
  b0():
    v54 = allocate
    store Field 4 at v54
    inc_rc [u32 1, u32 2, u32 3, u32 4]
    v56 = allocate
    store [u32 1, u32 2, u32 3, u32 4] at v56
    jmp b2()
  b2():
    jmpif u1 0 then: b3, else: b4
  b3():
    store Field 4 at v54
    store [u32 1, u32 2, u32 3, u32 6] at v56
    jmp b4()
  b4():
    return 
}

We now just check blocks with jump terminators for stores as well.

ACIR Update

After the flattening pass, the following mem2reg pass may resolve a constant array into the array_get that gets inserted inside of merge_slice_values. This constant array can potentially be smaller than the constant index that gets specified during flattening if we are merging slices of two different sizes.

Inside ACIR when handling constant indices we now allow returning dummy data when we have a constant index that is greater than the array size. This should be safe to do as any slice accesses specified by the user will always codegen a check against the dynamic slice length. The only time we will have a raw array get for a slice like this is due to instructions inserted by the compiler such as with flattening.

Additional Context

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.

@vezenovm vezenovm changed the title fix(ssa): Slice not tracker in outer block fix(ssa): Slice not tracking in outer blocks correctly Feb 1, 2024
@vezenovm vezenovm changed the title fix(ssa): Slice not tracking in outer blocks correctly fix(ssa): Tracking slices stores correctly for parent blocks Feb 1, 2024
@vezenovm
Copy link
Contributor Author

vezenovm commented Feb 1, 2024

In this PR for merging nested slices #3979 the slice capacity tracker that was written for the fill internal slices pass was used by the flattening pass to track slice capacities. This let us remove the back tracking done by the get_slice_length method and the outer_block_stores field in flattening. I was hoping the simple fix I just pushed would be a quick bandaid but it looks to be causing other issues with more complex examples.

So, I am converting this PR back to a draft. I wanted to bring back the capacity tracker anyway to remove outer_block_stores, but now I will try it as a part of this PR.

EDIT: Going to do the capacity_tracker in a followup to this PR. It turns out this bug required two fixes. One for accurately tracking slices in parent blocks which could be done with the simple bandaid in this PR, and then ACIR updates outlined in the main PR description.

@vezenovm vezenovm marked this pull request as draft February 1, 2024 04:34
AztecBot pushed a commit that referenced this pull request Feb 1, 2024
…nition (#4221)

Resolves #4222

Currently in order to specify whether we want to use a prover that
produces SNARK recursion friendly proofs, we must pass a flag from the
tooling infrastructure. This PR moves it be part of the circuit
definition itself.

The flag now lives on the Builder and is set when we call
`create_circuit` in the acir format. The proof produced when this flag
is true should be friendly for recursive verification inside of another
SNARK. For example, a recursive friendly proof may use Blake3Pedersen
for hashing in its transcript, while we still want a prove that uses
Keccak for its transcript in order to be able to verify SNARKs on
Ethereum.

However, a verifier does not need a full circuit description and should
be able to verify a proof with just the verification key and the proof.
An `is_recursive_circuit` field was thus added to the verification key
as well so that we can specify the accurate verifier to use for a given
proof without the full circuit description.

---------

Signed-off-by: kevaundray <[email protected]>
Co-authored-by: ledwards2225 <[email protected]>
Co-authored-by: kevaundray <[email protected]>
@vezenovm vezenovm marked this pull request as ready for review February 1, 2024 22:34
@vezenovm vezenovm changed the title fix(ssa): Tracking slices stores correctly for parent blocks fix(ssa): Tracking slices stores correctly for parent blocks and handling in ACIR Feb 1, 2024
@vezenovm vezenovm marked this pull request as draft February 2, 2024 17:52
@vezenovm
Copy link
Contributor Author

vezenovm commented Feb 2, 2024

Have one more idea I want to try out to avoid the ACIR changes so converting back to a draft temporarily.

@vezenovm
Copy link
Contributor Author

vezenovm commented Feb 2, 2024

Have one more idea I want to try out to avoid the ACIR changes so converting back to a draft temporarily.

I was able to resolve this by fixing up #4240 and making a truly accurate slice capacity tracker. Closing this PR.

@vezenovm vezenovm closed this Feb 2, 2024
@TomAFrench TomAFrench deleted the mv/fix-4202 branch November 20, 2024 12:03
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.

Compiler panic when a slice is modified in multiple if statements.
1 participant