chore: add acir-gen unit tests per ssa instruction#8084
chore: add acir-gen unit tests per ssa instruction#8084
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ 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: f3b2941 | Previous: b591847 | Ratio |
|---|---|---|---|
rollup-merge |
0.004 s |
0.003 s |
1.33 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
There was a problem hiding this comment.
⚠️ 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: f3b2941 | Previous: b591847 | Ratio |
|---|---|---|---|
test_report_AztecProtocol_aztec-packages_noir-projects_noir-protocol-circuits_crates_blob |
51 s |
41 s |
1.24 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
vezenovm
left a comment
vezenovm
left a comment
There was a problem hiding this comment.
I was thinking of ACIR gen unit tests that were more similar to our SSA units tests. Something along the lines of we accept an SSA src text and check that it matches the supplied ACIR text. We would probably need to complete this issue #7902 and make an ACIR parser similar to the SSA parser to do that
| .expect("Should compile manually written SSA into ACIR"); | ||
| assert_eq!(acir_functions.len(), 1); | ||
| let main = &acir_functions[0]; | ||
| let mut acvm = ACVM::new( |
There was a problem hiding this comment.
Executing in the acir-gen unit tests feels a little strange to me. It feels more like an integration test versus a test of the output of our ACIR gen.
There was a problem hiding this comment.
For me it is a unit test because it test a single operator.
Sounds good, agreed. In addition to your note, we already do test in this way for the existing ACIR gen unit tests. Either way I still think the ACIR gen unit tests should be more in the vein mentioned in my original comment. e.g. I mentioned completing #7902 and an ACIR parser as it may be worth it to still complete that task before adding more unit tests as it would not only make the unit tests more binding but quicker to write. If we have tests we can add now though without completing #7902 though I won't block those. |
vezenovm
left a comment
vezenovm
left a comment
There was a problem hiding this comment.
Looks good! I am more comfortable with these tests being under acir now that they use the SSA interpreter vs. constant folding. I think ideally these would be moved into their own interpreter module or something, but as this PR has been up for a while I am good with the PR as is. The tests can reorganized in follow-ups. Just some last nits with regards to comments.
| "truncate 8 32", | ||
| ]; | ||
| let inputs = [FieldElement::from(2_usize), FieldElement::from(1_usize)]; | ||
| test_operators(&operators, "u32", &inputs); |
There was a problem hiding this comment.
I guess in follow-ups we can test the remaining numeric types then.
| // Bitwise operations are not allowed on field elements | ||
| // SSA interpreter will emit an error but not ACVM | ||
| // "and", | ||
| // "xor", |
There was a problem hiding this comment.
We can follow-up with expected error tests that would have corresponding issues.
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Compilation Time'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.
| Benchmark suite | Current: 740428d | Previous: ba05d72 | Ratio |
|---|---|---|---|
private-kernel-reset |
10.306 s |
8.076 s |
1.28 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @TomAFrench
Changes to Brillig bytecode sizes
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
Changes to number of Brillig opcodes executed
🧾 Summary (10% most significant diffs)
Full diff report 👇
|
|
Closing as duplicate. |
4 participants
Description
Problem*
Increase test coverage for acir-gen
Summary*
Add a framework for acir-gen unit tests.
It generates SSA for a single instruction, converts it using acir-gen and execute it.
Then we compare the result with a SSA evaluation using constant folding.
All 'simple' instructions are covered, in particular all the binary instructions.
More tests with different types or missing instructions such as array operations and calls could be added in future.
Additional Context
Documentation*
Check one:
PR Checklist*
cargo fmton default settings.