Skip to content

feat(testing): Add SSA interpreter for testing SSA#8115

Merged
jfecher merged 33 commits intomasterfrom
jf/ssa-interpreter
May 1, 2025
Merged

feat(testing): Add SSA interpreter for testing SSA#8115
jfecher merged 33 commits intomasterfrom
jf/ssa-interpreter

Conversation

@jfecher
Copy link
Contributor

@jfecher jfecher commented Apr 16, 2025

Description

Problem*

Resolves #6826

Summary*

Implements an SSA interpreter purely for debugging purposes.

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] 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 Apr 16, 2025

The interpreter is currently in a very incomplete yet semi-working state so I thought I'd put this up to see if anyone had any comments or suggestions.

Going forward I think most of the remaining work will be:

  • Implementing each builtin
  • Threading errors throughout the interpreter instead of panics
  • Continuing to write more tests
  • Ways to invoke the interpreter from the CLI instead of just via unit tests.
    • Run each test_program against the ssa interpreter automatically and ensure values match?
    • CLI option to run the interpreter after each SSA pass to catch if a pass ever changes the result of the program.
    • Debugger-like options for the interpreter? E.g. "give me the value of v203." This one at least is out of scope for this PR.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Execution Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 2872cec Previous: 934126b Ratio
rollup-merge 0.004 s 0.003 s 1.33

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

Copy link
Contributor Author

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

This PR should be ready for review now. Some things to note:

  • ~2.3k lines of this PR are just tests in ssa/interpreter/tests/mod.rs and ssa/interpreter/tests/instructions.rs.
  • The interpreter currently still panics instead of returning error variants on error. We should update this later but I'm considering this ok for an MVP for testing.
  • A few intrinsics are still not implemented but again I'd rather leave them out of the MVP. These are:
    • Intrinsic::ApplyRangeConstraint we could just apply a range constraint but there's no documentation on any of the intrinsics so I'm not sure argument ordering, etc.
    • Intrinsic::BlackBox(_) all black box functions are unimplemented
    • Intrinsic::Hint(_) unsure what to do here
    • DerivePedersenGenerators this one was just a lot of code
    • We should implement these in follow-ups. I'll make an issue.
  • There are some necessary differences between this and SSA. Notably assert_constant is always true. I had it failing before on non-constant ValueIds but this meant we'd fail the same SSA on different optimization levels which is undesirable. static_assert is similar but just functions exactly like assert now.
  • The interpreter can still be used for unit testing and can be easily fed inputs and outputs. Constrains in the code are still asserted, etc.
  • I removed the CLI code from this PR, we can add this in a later PR so that it can be invoked via e.g. nargo interpret-ssa. Embedding it in other commands like nargo execute isn't ideal since we'd need to pass the program arguments down to noirc_evaluator.

@jfecher jfecher requested a review from a team April 29, 2025 19:50
@jfecher jfecher marked this pull request as ready for review April 29, 2025 19:50
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Test Suite Duration'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 0029fef Previous: 4bc636a Ratio
test_report_zkpassport_noir_rsa_ 30 s 22 s 1.36

This comment was automatically generated by workflow using github-action-benchmark.

CC: @TomAFrench

@aakoshh aakoshh mentioned this pull request Apr 30, 2025
Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Nice, looking forward to finding ways to use this with the fuzzer as an additional pillar of oracles!

@jfecher jfecher requested a review from aakoshh May 1, 2025 12:36
Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Awesome :)

This is going to be be a great reference in itself for what the behaviour should be 👍

@jfecher jfecher enabled auto-merge May 1, 2025 13:14
@jfecher jfecher added this pull request to the merge queue May 1, 2025
Merged via the queue into master with commit dc84c5d May 1, 2025
115 checks passed
@jfecher jfecher deleted the jf/ssa-interpreter branch May 1, 2025 14:22
github-merge-queue bot pushed a commit to AztecProtocol/aztec-packages that referenced this pull request May 2, 2025
Automated pull of nightly from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: disallow emitting multiple `MemoryInit` opcodes for the same block
(noir-lang/noir#8291)
fix(ssa): Remove unused calls to pure functions
(noir-lang/noir#8298)
fix(ssa): Do not remove unused checked binary ops
(noir-lang/noir#8303)
fix: Return zero and insert an assertion if RHS bit size is over the
limit in euclidian division
(noir-lang/noir#8294)
feat: remove unnecessary dynamic arrays when pushing onto slices
(noir-lang/noir#8287)
feat(testing): Add SSA interpreter for testing SSA
(noir-lang/noir#8115)
END_COMMIT_OVERRIDE

Co-authored-by: AztecBot <tech@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.

Add SSA interpreter for debugging

2 participants