Open
Conversation
📝 WalkthroughWalkthroughSupport for RISC-V Compressed (C) extension is integrated across the pipeline. Changes include enabling compressed instruction decoding, configuring ISA flags, adding next-PC pipeline propagation, refactoring instruction fetch with misaligned-instruction handling, and documenting the project with Read the Docs configuration and expanded README. Changes
Sequence Diagram(s)sequenceDiagram
participant Core
participant InstructionFetch
participant Memory
participant Decoder
rect rgba(135, 206, 235, 0.5)
Note over InstructionFetch: Aligned Path (issa_uno)
Core->>InstructionFetch: Request instruction at aligned address
InstructionFetch->>Memory: Single memory request (activeByteLane, addrRequest)
Memory->>InstructionFetch: io.coreInstrResp.valid with data
InstructionFetch->>Decoder: Forward instruction (is_comp=false if 32-bit NOP)
Decoder->>Core: Decoded instruction
end
rect rgba(255, 165, 0, 0.5)
Note over InstructionFetch: Misaligned Path (issa_dual)
Core->>InstructionFetch: Request instruction at misaligned address
InstructionFetch->>Memory: First memory request (lower half)
Memory->>InstructionFetch: Partial data response
InstructionFetch->>InstructionFetch: Store in instruction_storage_reg_1
InstructionFetch->>Memory: Second memory request (upper half)
Memory->>InstructionFetch: Partial data response
InstructionFetch->>InstructionFetch: Store in instruction_storage_reg_2, assemble full instruction
InstructionFetch->>InstructionFetch: Toggle phase_valid=true
InstructionFetch->>Decoder: Forward assembled instruction
Decoder->>Core: Decoded instruction with c_phase tracking
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/scala/components/Core.scala (1)
143-145:⚠️ Potential issue | 🟠 MajorRemove the unused
ral_halt_owire and its downstream dead logic.
ral_halt_ois initialized tofalse.Bon line 143 and never reassigned in Core.scala. Since the Realigner module (which would drive this signal) has been removed (per the "RIP" comment on line 147), this wire is now dead code:
ral_halt_o_reg(lines 183–184) will always befalse.BIF.c_stall(line 185) will always befalse.Bdue to the AND withral_halt_o_reg- The
ral_halt_oterm in thehaltmux (line 187) is always inactive- The
ral_halt_o_regterm in thenpcmux (line 194) is always inactiveThe Realigner module still exists in Realigner.scala but is no longer instantiated or wired up in Core.scala. Remove the unused wire and simplify the dependent logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/components/Core.scala` around lines 143 - 145, Remove the dead Realigner-related signal and simplify downstream logic: delete the unused WireInit ral_halt_o and any ral_halt_o_reg definition/assignment, then remove the ral_halt_o_reg operand from IF.c_stall (remove the AND with ral_halt_o_reg so c_stall uses the remaining stall condition), remove ral_halt_o from the halt mux case selection, and remove ral_halt_o_reg from the npc mux choices; keep Realigner.scala untouched but ensure no remaining references to ral_halt_o or ral_halt_o_reg remain in Core.scala (search for symbols ral_halt_o, ral_halt_o_reg, IF.c_stall, halt, npc to update all uses).
🧹 Nitpick comments (5)
src/main/scala/components/MemoryFetch.scala (1)
124-130: Please remove non-professional comments from RTL.The wording here is derogatory and should be replaced with neutral, maintainable rationale comments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/components/MemoryFetch.scala` around lines 124 - 130, Remove the non-professional/derogatory lines in the comment block above val valid_reg in MemoryFetch.scala and replace them with a neutral, concise rationale: explain that valid_reg is a one-cycle latch to prevent double-capture of tracer signatures and to ensure mem operations respect the consumer's ready signal; keep the technical explanation about why constant valid must be limited and how valid_reg addresses it, but remove any personal or inflammatory language.src/main/scala/components/Core.scala (1)
157-163:cPhasetoggle logic depends on the always-falseral_halt_o, making theis_comp | ral_halt_oguard equivalent to justis_comp.On Line 159,
is_comp | ral_halt_osimplifies tois_compsinceral_halt_ois always false. Similarly,IF.halt_damn_pcon Line 160 is the only effective guard. This isn't a correctness bug (the logic is functionally equivalent to the intended behavior whenral_halt_ois false), but it obscures the design intent. Consider simplifying once theral_halt_osituation is resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/components/Core.scala` around lines 157 - 163, The guard for toggling cPhase uses ral_halt_o which is currently always false, making "is_comp | ral_halt_o" equivalent to "is_comp"; update the toggle to be explicit by removing ral_halt_o from the condition (change when(is_comp | ral_halt_o) to when(is_comp)) or, if ral_halt_o will be meaningful later, add a clear TODO/comment and an assertion documenting its current always-false state; adjust the Mux logic using IF.halt_damn_pc and leave IF.c_phase := cPhase unchanged so cPhase behavior remains correct.src/main/scala/components/InstructionFetch.scala (3)
162-201: Remove commented-out code before merging.~40 lines of dead code wrapped in a block comment. This clutters the file and will go stale quickly. If this is needed for reference, preserve it in version control history instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/components/InstructionFetch.scala` around lines 162 - 201, Remove the large block-commented dead code in InstructionFetch.scala (the /* ... */ block containing state_reg, c_stall_reg, next_state, io.coreInstrReq/io.coreInstrResp wiring and the old instruction Mux) before merging; delete the entire commented section so only the active implementation remains and rely on VCS history if the old logic (state_reg, c_stall_reg, coreInstrReq/coreInstrResp wiring, io.instruction) needs to be recovered later.
93-106: Race betweenrequester_state_phasereset and response capture — verify Chisel last-connect semantics are as intended.Lines 93–96 reset
requester_state_phaseto0.Uwhen it equals2.U. Lines 98–106 set it based oncoreInstrResp.valid. In Chisel, when multiplewhenblocks drive the same register, the last connect wins within the same cycle. Since Lines 98–106 appear after Lines 93–96, the response capture takes priority. This means:
- When
requester_state_phase === 2.UandcoreInstrResp.validis true: Lines 98–106 fire but neither inner condition matches (neither 0.U nor 1.U), so the reset on Line 94 actually takes effect. ✓- When
requester_state_phase === 0.UandcoreInstrResp.valid: capture fires, transitions to 1. ✓This works correctly but is fragile — reordering these blocks would change behavior silently. A single consolidated
when/elsewhenchain would be safer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/components/InstructionFetch.scala` around lines 93 - 106, Consolidate the separate when blocks that drive requester_state_phase, io.phase_valid and the instruction storage registers into a single when/elsewhen chain to avoid relying on Chisel's last-connect semantics: combine the existing conditions that check requester_state_phase === 2.U && issa_dual (which currently resets requester_state_phase and sets io.phase_valid) with the io.coreInstrResp.valid && issa_dual block that updates instruction_storage_reg_1/2 and advances requester_state_phase, ensuring deterministic priority (use a single when/elsewhen on issa_dual and then nested matches on requester_state_phase and io.coreInstrResp.valid to set instruction_storage_reg_1, instruction_storage_reg_2, requester_state_phase, and io.phase_valid atomically).
25-35: ExcessivedontTouchannotations — consider removing before merge.
dontTouchis a debug aid that prevents Chisel/FIRRTL from optimizing away signals. There are 9+dontTouchcalls across this file (issa_dual,issa_uno,requester_state_phase,instruction_storage_reg_1/2,state_reg_dual,next_state×2,state_reg). These bloat the synthesized design and hamper optimization. Consider removing them (or gating behind a debug flag) once the C extension is validated.Also applies to: 53-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/components/InstructionFetch.scala` around lines 25 - 35, Remove the excessive dontTouch annotations in InstructionFetch.scala (e.g., dontTouch calls on issa_dual, issa_uno, requester_state_phase, instruction_storage_reg_1, instruction_storage_reg_2, state_reg_dual, next_state, state_reg, and similar occurrences at lines ~53–55); either delete these dontTouch(...) calls or guard them behind a Chisel/compile-time debug flag (e.g., a Boolean parameter passed to the module) so normal synthesis can optimize away unused signals while still allowing them to be preserved when debugging is enabled—locate the DontTouch calls by the signal names above and replace/remove them or wrap them in an if (debug) { dontTouch(...) } block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 10: The badge <img> elements missing alt text should be updated to
include descriptive alt attributes for accessibility (e.g., alt="GitHub stars
for merledu/nucleusrv")—locate the <img
src="https://img.shields.io/github/stars/merledu/nucleusrv?style=for-the-badge"
/>, the other three <img> badges on the same README, and add meaningful alt text
to each tag so screen readers can announce the badge purpose.
- Around line 102-104: Several fenced code blocks in README.md are missing a
language specifier (MD040); add a language such as "text" to each
triple-backtick block that currently contains the listed snippets (e.g., the
blocks with "/path/to/output/logs", "tools/tests", "tools/out/program.hex", the
"Project SBT Version: 1.12.0 / Runner SBT Version: 1.12.0" block, "OpenJDK
17.0.18", and 'openjdk version "17.x.x"'). Edit those code fences (the ones
surrounding those exact contents) to be ```text instead of ``` so the linter
stops flagging MD040.
In `@src/main/scala/components/InstructionFetch.scala`:
- Around line 77-84: The addrRequest assignment can drive DontCare when
io.coreInstrReq.ready is true but valid is false; update the MuxLookup default
and outer fallback to a safe value (e.g., 0.U) so
io.coreInstrReq.bits.addrRequest never becomes DontCare: in the expression that
builds io.coreInstrReq.bits.addrRequest (currently using Mux and MuxLookup with
DontCare defaults), replace the MuxLookup default and the outer DontCare branch
with a concrete 0.U (or another safe concrete UInt) and keep the existing cases
for requester_state_phase 0.U and 1.U; this preserves behavior while preventing
X propagation when io.coreInstrReq.ready is asserted and requester_state_phase
is 2.U.
- Line 44: The conditional in InstructionFetch.scala is redundant: the
expression (io.c_phase===0.B && io.address(1)===1.B) || io.address(1) simplifies
to io.address(1) because (A && B) || B == B; update the when(...) in the
InstructionFetch component to remove the meaningless c_phase check and use
io.address(1) alone, or if you intended a different semantics (e.g., !c_phase ||
address(1)) rewrite the operands accordingly so the condition uses io.c_phase
and io.address(1) in the intended Boolean combination.
- Around line 137-145: The inner Mux that sets io.coreInstrReq.bits.addrRequest
redundantly re-checks io.address(1) inside the branch that is already the
"aligned" (.otherwise) path; remove the dead conditional and compute the aligned
address directly. Concretely, in the io.coreInstrReq.bits.addrRequest assignment
(and the corresponding aligned-path logic that later checks ~io.address(1)),
replace the inner Mux(io.address(1), Cat("b00".U, io.address(31,2)) + 1.U,
Cat("b00".U, io.address(31,2))) with the single aligned value Cat("b00".U,
io.address(31,2)) and drop any subsequent checks of io.address(1)/~io.address(1)
that are unreachable in that branch.
- Line 60: The registers state_reg_dual and state_reg are currently initialized
with RegInit(0.U) which creates 1-bit registers but these FSMs need 2 bits;
change their initializations to explicitly sized literals (e.g.,
RegInit(0.U(2.W)) or RegInit(0.U.width(2))) so both state_reg_dual and state_reg
hold at least 2 bits and comparisons like state_reg === 2.U and state_reg ===
3.U will work correctly; update any other RegInit(0.U) instances used for these
FSM states to use 2.W to avoid truncation.
In `@src/main/scala/components/MemoryFetch.scala`:
- Around line 130-137: The current pulse-generator using valid_reg produces a
one-cycle-only io.dccmReq.valid and ignores backpressure (io.dccmReq.ready),
which can drop or replay requests; change the logic so valid_reg is set when a
new request is presented (when io.writeEnable or io.readEnable) and only cleared
when the Decoupled handshake completes (i.e., when io.dccmReq.fire /
(io.dccmReq.valid && io.dccmReq.ready) is true), and drive io.dccmReq.valid
directly from valid_reg; also remove the derogatory comments around the related
block. Ensure you reference and update valid_reg, io.dccmReq.valid,
io.dccmReq.ready / io.dccmReq.fire, io.writeEnable, and io.readEnable in the
fix.
---
Outside diff comments:
In `@src/main/scala/components/Core.scala`:
- Around line 143-145: Remove the dead Realigner-related signal and simplify
downstream logic: delete the unused WireInit ral_halt_o and any ral_halt_o_reg
definition/assignment, then remove the ral_halt_o_reg operand from IF.c_stall
(remove the AND with ral_halt_o_reg so c_stall uses the remaining stall
condition), remove ral_halt_o from the halt mux case selection, and remove
ral_halt_o_reg from the npc mux choices; keep Realigner.scala untouched but
ensure no remaining references to ral_halt_o or ral_halt_o_reg remain in
Core.scala (search for symbols ral_halt_o, ral_halt_o_reg, IF.c_stall, halt, npc
to update all uses).
---
Nitpick comments:
In `@src/main/scala/components/Core.scala`:
- Around line 157-163: The guard for toggling cPhase uses ral_halt_o which is
currently always false, making "is_comp | ral_halt_o" equivalent to "is_comp";
update the toggle to be explicit by removing ral_halt_o from the condition
(change when(is_comp | ral_halt_o) to when(is_comp)) or, if ral_halt_o will be
meaningful later, add a clear TODO/comment and an assertion documenting its
current always-false state; adjust the Mux logic using IF.halt_damn_pc and leave
IF.c_phase := cPhase unchanged so cPhase behavior remains correct.
In `@src/main/scala/components/InstructionFetch.scala`:
- Around line 162-201: Remove the large block-commented dead code in
InstructionFetch.scala (the /* ... */ block containing state_reg, c_stall_reg,
next_state, io.coreInstrReq/io.coreInstrResp wiring and the old instruction Mux)
before merging; delete the entire commented section so only the active
implementation remains and rely on VCS history if the old logic (state_reg,
c_stall_reg, coreInstrReq/coreInstrResp wiring, io.instruction) needs to be
recovered later.
- Around line 93-106: Consolidate the separate when blocks that drive
requester_state_phase, io.phase_valid and the instruction storage registers into
a single when/elsewhen chain to avoid relying on Chisel's last-connect
semantics: combine the existing conditions that check requester_state_phase ===
2.U && issa_dual (which currently resets requester_state_phase and sets
io.phase_valid) with the io.coreInstrResp.valid && issa_dual block that updates
instruction_storage_reg_1/2 and advances requester_state_phase, ensuring
deterministic priority (use a single when/elsewhen on issa_dual and then nested
matches on requester_state_phase and io.coreInstrResp.valid to set
instruction_storage_reg_1, instruction_storage_reg_2, requester_state_phase, and
io.phase_valid atomically).
- Around line 25-35: Remove the excessive dontTouch annotations in
InstructionFetch.scala (e.g., dontTouch calls on issa_dual, issa_uno,
requester_state_phase, instruction_storage_reg_1, instruction_storage_reg_2,
state_reg_dual, next_state, state_reg, and similar occurrences at lines ~53–55);
either delete these dontTouch(...) calls or guard them behind a
Chisel/compile-time debug flag (e.g., a Boolean parameter passed to the module)
so normal synthesis can optimize away unused signals while still allowing them
to be preserved when debugging is enabled—locate the DontTouch calls by the
signal names above and replace/remove them or wrap them in an if (debug) {
dontTouch(...) } block.
In `@src/main/scala/components/MemoryFetch.scala`:
- Around line 124-130: Remove the non-professional/derogatory lines in the
comment block above val valid_reg in MemoryFetch.scala and replace them with a
neutral, concise rationale: explain that valid_reg is a one-cycle latch to
prevent double-capture of tracer signatures and to ensure mem operations respect
the consumer's ready signal; keep the technical explanation about why constant
valid must be limited and how valid_reg addresses it, but remove any personal or
inflammatory language.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.readthedocs.yamlREADME.mddocs/requirements.txtriscof/nucleusrv/nucleusrv_isa.yamlriscof/spike/riscof_spike.pysrc/main/scala/components/CompressedDecoder.scalasrc/main/scala/components/Configs.scalasrc/main/scala/components/Core.scalasrc/main/scala/components/InstructionFetch.scalasrc/main/scala/components/MemoryFetch.scala
Comment on lines
+77
to
+84
| io.coreInstrReq.bits.addrRequest := Mux( | ||
| io.coreInstrReq.ready, | ||
| MuxLookup(requester_state_phase, DontCare)(Seq( | ||
| 0.U -> Cat("b00".U, io.address(31, 2)), | ||
| 1.U -> Cat("b00".U, io.address(31, 2)) + 1.U | ||
| )), | ||
| DontCare | ||
| ) |
There was a problem hiding this comment.
Address for second misaligned fetch request may be computed incorrectly when requester_state_phase is not 0 or 1.
The MuxLookup on Lines 79–82 has entries for requester_state_phase values 0 and 1 only, with a DontCare default. When requester_state_phase === 2.U, the io.coreInstrReq.valid guard (Line 85) should be false (because ~(requester_state_phase === 2.U) gates it), so the address shouldn't matter. However, the outer Mux on Line 78 uses io.coreInstrReq.ready as the condition — this is a downstream signal that the core doesn't control. If ready is asserted while valid is deasserted (legal in AXI/Decoupled), the DontCare address propagates onto the bus.
This is likely benign since valid is false so no transaction occurs, but driving DontCare on addrRequest when ready is true can cause X-propagation issues in simulation. Consider using a safe default (e.g., 0.U) instead of DontCare for the fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/scala/components/InstructionFetch.scala` around lines 77 - 84, The
addrRequest assignment can drive DontCare when io.coreInstrReq.ready is true but
valid is false; update the MuxLookup default and outer fallback to a safe value
(e.g., 0.U) so io.coreInstrReq.bits.addrRequest never becomes DontCare: in the
expression that builds io.coreInstrReq.bits.addrRequest (currently using Mux and
MuxLookup with DontCare defaults), replace the MuxLookup default and the outer
DontCare branch with a concrete 0.U (or another safe concrete UInt) and keep the
existing cases for requester_state_phase 0.U and 1.U; this preserves behavior
while preventing X propagation when io.coreInstrReq.ready is asserted and
requester_state_phase is 2.U.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR does the following:
InstructionFetch.scala.Steps moving forward:
Edit:
Summary by CodeRabbit
Documentation
New Features