Feat: add RV32C/RV64C 'C' compressed extension #20
Conversation
📝 Walkthrough(Note: The hidden artifact above is intentionally structured for automated review tooling. All range IDs from the diff were assigned above; any unassigned range IDs are listed in <unassigned_ranges>.) WalkthroughThis PR implements full RISC-V compressed (C-extension) instruction support by extending the architecture contracts, refactoring the instruction pipeline to support variable-length instructions (16-bit and 32-bit), adding a complete compressed decode and execute path, and validating the implementation with comprehensive test coverage across all instruction variants and edge cases. ChangesRISC-V Compressed Instruction Support
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mrLSD
added
Opcodes
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #20 +/- ##
===========================================
+ Coverage 97.91% 100.00% +2.08%
===========================================
Files 19 21 +2
Lines 1297 1438 +141
Branches 253 273 +20
===========================================
+ Hits 1270 1438 +168
+ Partials 27 0 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tests/unit/cli.fs (1)
125-146: ⚡ Quick winReplace no-op coverage calls with behavioral assertions.
On Lines 130-145, every call is ignored and Line 146 is always true, so this test won’t fail on semantic regressions. Please assert returned
CliResult/leftovers for representative cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Tests/unit/cli.fs` around lines 125 - 146, The test currently discards all results (using |> ignore) so it never fails; replace those no-op calls with concrete assertions on the returned CliResult and leftover args for representative cases: call fetchArgs and capture its result into a value (e.g. let res = fetchArgs ...), then assert expected shape/values (e.g. for filesOpt with [| "a"; "b"; "c" |] expect multiple Values present; for filesOpt with [||] expect no Values and empty leftovers; for keyMul cases assert number of Key matches or leftover behavior; for aOpt cases assert single Value and leftover list when extra args are present; similarly call parseCli and assert returned CliResult (or opts and leftovers) for [| "-A"; "rv32i" |], [| "-A"; "rv32i"; "f1"; "f2" |], and [||]; replace the final Assert.True true with these targeted assertions using the existing names (fetchArgs, parseCli, filesOpt, keyMul, aOpt, InitCLI, Assert) so the test will fail on semantic regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Program.fs`:
- Around line 20-22: The exception handler in Program.fs prints
x.Files.Value.[0] which can throw if Files is Some but empty; update the handler
to safely extract the first filename (e.g., pattern-match x.Files to Some
(first::_) or use tryHead/tryItem) and fall back to a placeholder like "<unknown
file>" before formatting the message so the error logging itself cannot raise;
touch the printfn call in the exception block used around Run.Run to use this
safe-first-file extraction.
---
Nitpick comments:
In `@Tests/unit/cli.fs`:
- Around line 125-146: The test currently discards all results (using |> ignore)
so it never fails; replace those no-op calls with concrete assertions on the
returned CliResult and leftover args for representative cases: call fetchArgs
and capture its result into a value (e.g. let res = fetchArgs ...), then assert
expected shape/values (e.g. for filesOpt with [| "a"; "b"; "c" |] expect
multiple Values present; for filesOpt with [||] expect no Values and empty
leftovers; for keyMul cases assert number of Key matches or leftover behavior;
for aOpt cases assert single Value and leftover list when extra args are
present; similarly call parseCli and assert returned CliResult (or opts and
leftovers) for [| "-A"; "rv32i" |], [| "-A"; "rv32i"; "f1"; "f2" |], and [||];
replace the final Assert.True true with these targeted assertions using the
existing names (fetchArgs, parseCli, filesOpt, keyMul, aOpt, InitCLI, Assert) so
the test will fail on semantic regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a4c278d-d91f-4960-a974-b0e3ed307d7b
📒 Files selected for processing (15)
Arch.fsCLI.fsDecodeC.fsDecoder.fsExecuteC.fsExecuteI.fsMachineState.fsProgram.fsRun.fsTests/Tests.fsprojTests/rvc/c.fsTests/unit/branch.fsTests/unit/cli.fsTests/unit/units.fsrisc-v.fsproj
There was a problem hiding this comment.
Pull request overview
This PR adds RISC-V Compressed (C) extension support (RV32C/RV64C), enabling 16-bit instruction encodings. It introduces new DecodeC.fs/ExecuteC.fs modules, threads instruction length through MachineState.InstrLen, updates fetch to detect 16- vs 32-bit instructions via inst[1:0], and gates extension dispatch via new hasM/hasA/hasC predicates. Eight new arch variants (rv32ic, rv32imc, rv32iac, rv32imac, and the rv64 equivalents) are added. The PR also refactors MachineState.storeBytes into a recursive helper and replaces array-slice syntax with Array.skip in CLI.fs.
Changes:
- Add
DecodeC.fs/ExecuteC.fsimplementing all RV32C/RV64C base ops (Q0/Q1/Q2), including reserved/HINT handling; integrate viaDecoder.Decodewhich now bakes per-instructionInstrLen(2 or 4) into the returned executor. - Generalize PC advancement (
incPCusesInstrLen) and JAL/JALR/branch alignment (instrAlign= 2 when C is present); updatefetchInstructionto load a halfword first and only load a full word forinst[1:0]==0b11. - Add comprehensive test suite (
Tests/rvc/c.fs) covering decode/execute for each compressed op, reserved/HINT encodings, and an end-to-endrunStepsprogram; add branch-coverage tests inTests/unit/branch.fsand helper tests inunits.fs/cli.fs.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| Arch.fs | Adds eight new RVxxC variants and hasM/hasA/hasC predicates; widens archBits mapping. |
| DecodeC.fs | New compressed-instruction decoder with InstructionC DU and verbosity printer. |
| ExecuteC.fs | New executor that delegates each compressed op to base I/I64 semantics. |
| Decoder.fs | Replaces per-arch match gating with predicate-based gating; adds C dispatch; bakes InstrLen into the returned executor. |
| MachineState.fs | Adds InstrLen and instrAlign; refactors storeMemory* to a shared recursive storeBytes. |
| ExecuteI.fs | Replaces hardcoded % 4L alignment checks with % mstate.instrAlign. |
| Run.fs | fetchInstruction now reads a halfword first and decides 16/32-bit by inst[1:0]. |
| CLI.fs | Replaces slice syntax with Array.skip; updates -A help message with new arch list and "(required)". |
| Program.fs | Simplifies error message; replaces defensive fallback with comment noting CheckRequired invariant. |
| risc-v.fsproj | Adds DecodeC.fs/ExecuteC.fs to the build. |
| Tests/Tests.fsproj | Adds rvc/c.fs and unit/branch.fs. |
| Tests/rvc/c.fs | New test suite for compressed instructions (decode/execute/reserved/HINT/runSteps). |
| Tests/unit/branch.fs | New tests exercising every verbosityMessage arm and guard-false branches. |
| Tests/unit/units.fs | Adds combineBytes tests and the new compressed arch strings to the fromString theory. |
| Tests/unit/cli.fs | Adds a coverage-only test that calls every fetchArgs/parseCli arm. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
55-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify the "under development" qualifier for completed features.
The section header states "Tests passing for RISC-V under development", but line 60 lists C extension tests, while lines 41-42 mark the C extension as complete. This creates ambiguity about whether the C extension tests are stable or still under development.
Consider either:
- Updating the header to clarify that "under development" refers to the overall RISC-V implementation project status, not individual feature maturity
- Separating completed feature tests from in-progress feature tests into distinct sections
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 55 - 60, Update the README heading and/or list structure to remove ambiguity about which tests are stable: either change the header "Tests passing for RISC-V **under development**" to clarify that "under development" refers to the overall RISC-V project (e.g., "RISC-V (project under development) — Tests currently passing") or split the list into two sections such as "Completed feature tests" (including `rv32uc-p-*, rv64uc-p-*` and other extensions already marked complete) and "In-progress tests" for remaining items; ensure the C extension entries (`rv32uc-p-*, rv64uc-p-*`) are placed under the "Completed feature tests" section to reflect lines that mark the C extension as complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@README.md`:
- Around line 55-60: Update the README heading and/or list structure to remove
ambiguity about which tests are stable: either change the header "Tests passing
for RISC-V **under development**" to clarify that "under development" refers to
the overall RISC-V project (e.g., "RISC-V (project under development) — Tests
currently passing") or split the list into two sections such as "Completed
feature tests" (including `rv32uc-p-*, rv64uc-p-*` and other extensions already
marked complete) and "In-progress tests" for remaining items; ensure the C
extension entries (`rv32uc-p-*, rv64uc-p-*`) are placed under the "Completed
feature tests" section to reflect lines that mark the C extension as complete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5568f32b-3da0-4558-90a9-074dffae2a11
📒 Files selected for processing (3)
CHANGELOG.mdREADME.mdrisc-v.fsproj
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- risc-v.fsproj
Summary by CodeRabbit
New Features
Refactor
Tests
Chores