-
Notifications
You must be signed in to change notification settings - Fork 5
Add RISC-V interpreter compliance testing framework and tests for RV64I and RV64M #534
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds a comprehensive RISC-V interpreter compliance testing framework that validates the interpreter against the official riscv-arch-test suite. The framework clones the test repository during build, parses test macros from assembly files, and executes them through the interpreter to verify correctness.
Changes:
- Added new crate
ab-riscv-interpreter-compliance-testswith build-time git cloning of the official RISC-V architecture test suite - Implemented test utilities for parsing and executing compliance tests for RV64I and RV64M extensions
- Modified the
Fenceinstruction to remove thefmfield from the enum representation while adding validation during decoding to ensurefmis 0 per RISC-V specification
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
crates/execution/ab-riscv-interpreter-compliance-tests/build.rs |
Clones the riscv-arch-test repository at build time with commit verification |
crates/execution/ab-riscv-interpreter-compliance-tests/Cargo.toml |
Package configuration with dependencies on interpreter, macros, and primitives |
crates/execution/ab-riscv-interpreter-compliance-tests/src/lib.rs |
Library root defining module structure and exposing repo path constant |
crates/execution/ab-riscv-interpreter-compliance-tests/src/rv64.rs |
Defines FullRv64BInstruction enum for future B extension testing support |
crates/execution/ab-riscv-interpreter-compliance-tests/tests/rv64/utils.rs |
Core testing utilities including memory implementation, instruction fetching, and test macro parsers |
crates/execution/ab-riscv-interpreter-compliance-tests/tests/rv64/main.rs |
Integration tests for RV64I and RV64M extensions with instruction mapping |
crates/execution/ab-riscv-primitives/src/instruction/rv64.rs |
Removes fm field from Fence enum and adds validation during decoding |
crates/execution/ab-riscv-primitives/src/instruction/rv64/tests.rs |
Updates Fence instruction tests to reflect API changes and add validation tests |
crates/execution/ab-riscv-interpreter/src/rv64/tests.rs |
Updates Fence test to remove fm field usage |
Cargo.lock |
Adds new compliance-tests crate to workspace dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
crates/execution/ab-riscv-primitives/src/instruction/rv64/tests.rs
Outdated
Show resolved
Hide resolved
crates/execution/ab-riscv-interpreter-compliance-tests/tests/rv64/utils.rs
Show resolved
Hide resolved
crates/execution/ab-riscv-interpreter-compliance-tests/tests/rv64/utils.rs
Outdated
Show resolved
Hide resolved
crates/execution/ab-riscv-interpreter-compliance-tests/tests/rv64/main.rs
Show resolved
Hide resolved
crates/execution/ab-riscv-primitives/src/instruction/rv64/tests.rs
Outdated
Show resolved
Hide resolved
fe865b3 to
2f808d9
Compare
2f808d9 to
c421bf2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
We basically clone https://github.com/riscv-non-isa/riscv-arch-test, parse its contents for instructions and execute them through an interpreter, ensuring correctness.
Unfortunately, test cases for B extension appear to be broken: riscv-non-isa/riscv-arch-test#860
The approach is quite hacky, but hopefully worth it. There is apparently changes coming in the architecture tests in the future, which might make this easier or harder to maintain.