Cycle delay fix#101
Open
5hayanB wants to merge 11 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR restructures NucleusRV's documentation with new guides, concepts, and references while extending hardware configurability. It makes atomic (A), floating-point (F), and CSR (Zicsr) extensions conditionally compilable through module parameterization, simplifies memory interface handshakes, and updates default feature configurations to disable M/F/Zicsr by default. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
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/Configs.scala (1)
3-13:⚠️ Potential issue | 🟡 MinorBreaking change: Default ISA has shifted from
RV32IAconfiguration (disabled M, F, and Zicsr extensions).This affects
Top.scala:14, which instantiatesConfigs()without parameters. The feature gates are properly implemented throughout the codebase—disabled extensions useOptionpatterns and don't instantiate unnecessary modules—so no compilation errors are expected.Consider adding a migration note or changelog entry documenting this default configuration change for users who rely on implicit
Configs()instantiation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/components/Configs.scala` around lines 3 - 13, Default ISA behavior changed because Configs default enables A and TRACE and disables M, F, Zicsr (so default is RV32IA with A true); update the project documentation/changelog and/or adjust instantiations that rely on implicit Configs() to explicitly pass desired flags. Specifically, add a migration note describing the new defaults for Configs (symbols: Configs, XLEN, M, F, Zicsr, A, TRACE) and either change callers that rely on implicit defaults (e.g., where Top constructs a Configs() without args) to pass explicit parameters or document that users must now instantiate Configs(M = true, F = true, Zicsr = true, ...) when expecting previous behavior.
🟡 Minor comments (4)
docs/installation.rst-31-34 (1)
31-34:⚠️ Potential issue | 🟡 MinorToolchain architecture mismatch with default ISA configuration.
The toolchain is configured with
--with-arch=rv32gcwhich includes M, F, D, C extensions, but the defaultConfigs.scalaproduces anRV32IAcore (only I and A extensions). This mismatch may confuse users or cause issues if code compiled with floating-point or compressed instructions is run on the default configuration.Consider either:
- Aligning the toolchain to
--with-arch=rv32iato match defaults, or- Adding a note explaining that
rv32gcis a superset and users should compile with appropriate-marchflags for their specific configuration.📝 Suggested documentation fix
- ./configure --prefix=/opt/riscv --with-arch=rv32gc --with-abi=ilp32 + ./configure --prefix=/opt/riscv --with-arch=rv32ia --with-abi=ilp32Or add explanatory text:
.. note:: The default NucleusRV configuration supports RV32IA. Adjust ``--with-arch`` to match your enabled extensions (e.g., ``rv32imfac`` if enabling M, F, and C).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/installation.rst` around lines 31 - 34, The docs currently recommend building the toolchain with --with-arch=rv32gc which includes M/F/D/C but the default Configs.scala produces an RV32IA core; update the installation instructions to either change the configure flag to --with-arch=rv32ia to match the default Configs.scala, or keep --with-arch=rv32gc but add a clear note that rv32gc is a superset and users must compile with appropriate -march flags (or choose a matching --with-arch like rv32imfc) for their enabled extensions; reference Configs.scala in the note so readers know where the default core is defined and show the suggested short note text advising matching toolchain arch to enabled extensions.docs/reference/pipeline-stages.rst-61-61 (1)
61-61:⚠️ Potential issue | 🟡 MinorUse one naming convention for the debug port in this page.
Line 61 uses
io.pinwhile Line 73 usesio_pin. Pick one form (or explicitly map Chisel vs Verilog naming) to avoid ambiguity.Also applies to: 73-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/pipeline-stages.rst` at line 61, The page mixes two names for the debug port (io.pin and io_pin); pick one convention and make it consistent across the page (or explicitly state a mapping between Chisel and Verilog names). Update every occurrence (e.g., the instances currently showing io.pin at line ~61 and io_pin at line ~73) to the chosen form or add a short note like “Chisel: io.pin ↔ Verilog: io_pin” and apply that consistently so readers aren’t confused.docs/reference/csr-map.rst-60-60 (1)
60-60:⚠️ Potential issue | 🟡 MinorUse consistent CSR name:
mcountinhibit(notmcountinh).Line 60 conflicts with the later heading (Lines 100-101) and can confuse readers when cross-referencing CSR docs.
🛠️ Proposed doc fix
-| 0x320 | mcountinh | Machine counter-inhibit register | +| 0x320 | mcountinhibit | Machine counter-inhibit register |Also applies to: 100-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/reference/csr-map.rst` at line 60, Replace the inconsistent CSR name "mcountinh" with the canonical "mcountinhibit" in the docs: update the table row that currently shows "| 0x320 | mcountinh | Machine counter-inhibit register |" and any other occurrences (including the heading around lines 100-101) so the CSR name is consistently "mcountinhibit" throughout docs/reference/csr-map.rst; ensure cross-references and anchors (if any) use the same term.docs/guides/running-assembly.rst-9-10 (1)
9-10:⚠️ Potential issue | 🟡 MinorKeep the source/output paths consistent.
The guide says
program.Slives undertools/, but the commands compileprogram.Sand readprogram.elffrom the current directory. Copy-pasting this from repo root will fail.Suggested doc fix
- riscv32-unknown-elf-gcc -march=rv32gcv -mabi=ilp32 -static -nostdlib -nostartfiles -T tools/link.ld program.S -o program.elf - python3 tools/makehex.py program.elf 2048 > imem.hex + riscv32-unknown-elf-gcc -march=rv32imafc_zicsr -mabi=ilp32f -static -nostdlib -nostartfiles -T tools/link.ld tools/program.S -o tools/program.elf + python3 tools/makehex.py tools/program.elf 2048 > imem.hexAlso applies to: 28-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guides/running-assembly.rst` around lines 9 - 10, The doc references program.S in the tools/ directory but subsequent commands operate on program.S and program.elf in the current directory, which will break copy/paste; update the guide so the paths are consistent (either change the example files to tools/program.S and tools/program.elf in the commands or move the initial sentence to indicate the file is in the repo root), and adjust all occurrences of "program.S" and "program.elf" (including the commands around lines 9–10 and 28–29) to use the same directory naming (e.g., tools/program.S and tools/program.elf) so the instructions work when run verbatim.
🧹 Nitpick comments (12)
riscof/nucleusrv/riscof_nucleusrv.py (1)
174-198: Consider fail-fast command chaining for post-processing steps.In both branches,
simcmduses;, so ifobjdump/objcopy/hexdumpfails, later commands still run and can hide artifact-generation failures. Prefer&&for this chain.Proposed refactor
- simcmd = '; '.join(( + simcmd = ' && '.join(( self.objcopy.format(self.xlen, elf, 'imem.bin', '.text.init'), self.objcopy.format(self.xlen, elf, 'dmem.bin', '.data'), self.hexdump.format('imem.bin', 'imem.hex'), self.hexdump.format('dmem.bin', 'dmem.hex'), self.objdump.format(self.xlen, elf), f'cd {self.dut}', self.sbt.format( os.path.join(testentry['work_dir'], 'imem.hex'), os.path.join(testentry['work_dir'], 'dmem.hex'), test_name ), f"cd {os.path.join('out', test_name)}", "(echo '/* verilator lint_off WIDTH */' && cat Top.v) > temp && mv temp Top.v", 'verilator --cc --exe --build --trace --no-timing ../../tb_Top.cpp Top.v', f'./obj_dir/VTop > {sig_file} 2>&1' )) ... - simcmd = '; '.join(( + simcmd = ' && '.join(( self.objcopy.format(self.xlen, elf, 'imem.bin', '.text.init'), self.objcopy.format(self.xlen, elf, 'dmem.bin', '.data'), self.hexdump.format('imem.bin', 'imem.hex'), self.hexdump.format('dmem.bin', 'dmem.hex'), self.objdump.format(self.xlen, elf) ))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@riscof/nucleusrv/riscof_nucleusrv.py` around lines 174 - 198, The command chains built into simcmd (in both branches where simcmd is assembled) use ';' which allows later steps to run even if earlier steps like objcopy, hexdump, or objdump fail; change the separators between those commands to '&&' so the chain fails fast and downstream steps (including the sbt/verilator/run steps) won't execute on earlier errors—update the constructions that call self.objcopy, self.hexdump, self.objdump and the branch that calls self.sbt/verilator to join commands with '&&' instead of ';'.src/main/resources/sram.v (2)
65-87: Remove commented-out legacy register path.Lines 65-87 keep dead implementation details inline and make active behavior harder to audit. Prefer deleting this block (or moving rationale to a short comment/changelog note).
Suggested cleanup
- //reg csb0_reg; - //reg web0_reg; - //reg [NUM_WMASKS-1:0] wmask0_reg; - //reg [ADDR_WIDTH-1:0] addr0_reg; - //reg [DATA_WIDTH-1:0] din0_reg; - //reg [DATA_WIDTH-1:0] dout0; - - // All inputs are registers - //always @(posedge clk0) - //begin - // csb0_reg = csb0; - // web0_reg = web0; - // wmask0_reg = wmask0; - // addr0_reg = addr0; - // din0_reg = din0; - //dout0 = 32'bx0; -/*`ifdef DBG - if ( !csb0_reg && web0_reg ) - $display($time," Reading %m addr0=%b dout0=%b",addr0_reg,mem[addr0_reg]); - if ( !csb0_reg && !web0_reg ) - $display($time," Writing %m addr0=%b din0=%b wmask0=%b",addr0_reg,din0_reg,wmask0_reg); -`endif -*/ //end + // Port-0 request signals are used directly (no input staging registers).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/sram.v` around lines 65 - 87, The file contains a large commented-out legacy register path (csb0_reg, web0_reg, wmask0_reg, addr0_reg, din0_reg, dout0 and the clk0 always block plus debug prints) that clutters the active design; remove this dead block or replace it with a single-line note pointing to the rationale/history (or a changelog entry) so the module only contains the current implementation and not inline legacy code. Ensure any needed debug behavior is documented elsewhere and no references to these symbols remain in active code.
118-126: Byte-lane writes are hard-coded to 32-bit/4-mask width.Lines 119-126 ignore parameterization (
NUM_WMASKS,DATA_WIDTH) and will misbehave for non-32-bit configurations.Suggested parameterized write-lane logic
always @ (negedge clk0) begin : MEM_WRITE0 if ( !csb0 && !web0 ) begin - if (wmask0[0]) - mem[addr0][7:0] = din0[7:0]; - if (wmask0[1]) - mem[addr0][15:8] = din0[15:8]; - if (wmask0[2]) - mem[addr0][23:16] = din0[23:16]; - if (wmask0[3]) - mem[addr0][31:24] = din0[31:24]; + integer b; + for (b = 0; b < NUM_WMASKS; b = b + 1) begin + if (wmask0[b]) + mem[addr0][8*b +: 8] = din0[8*b +: 8]; + end end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/sram.v` around lines 118 - 126, The byte-lane write logic is hard-coded for 4 masks/32-bit and must be parameterized: inside the same conditional (when !csb0 && !web0) replace the four explicit wmask0[x] checks with a loop over i from 0 to NUM_WMASKS-1 that checks wmask0[i] and assigns the corresponding slice of mem[addr0] from bit index high = (i+1)*(DATA_WIDTH/NUM_WMASKS)-1 down to low = i*(DATA_WIDTH/NUM_WMASKS) with din0[high:low]; ensure you use the NUM_WMASKS and DATA_WIDTH parameters to compute slice widths and that the loop variable and slicing match the net names (wmask0, mem, addr0, din0) so the code works for non-32-bit configurations.docs/installation.rst (1)
49-51: Incomplete sbt installation instructions.The placeholder comment may leave users without clear guidance. Consider providing at least the common Debian/Ubuntu commands or a direct link to the official sbt installation page.
📝 Example improvement
sudo apt-get install -y default-jdk - # Install sbt via official repositories - # ... (follow sbt installation instructions for your distro) + # Install sbt (Debian/Ubuntu) + echo "deb https://repo.scala-sbt.org/scalasbt/debian all main" | sudo tee /etc/apt/sources.list.d/sbt.list + curl -sL "https://keyserver.ubuntu.com/pks/lookup?op=get&search=0x99E82A75642AC823" | sudo apt-key add + sudo apt-get update + sudo apt-get install -y sbt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/installation.rst` around lines 49 - 51, Replace the placeholder sbt installation comment with concrete instructions and a reference link: add the common Debian/Ubuntu installation commands for sbt (install apt-transport-https, add the sbt repository GPG key and repository, apt-get update, apt-get install sbt) and include a direct link to the official sbt installation page so users on other distros can follow vendor guidance; update the docs/installation.rst section that currently contains the "# Install sbt via official repositories" comment to include these commands and the URL.docs/requirements.txt (1)
1-3: Consider pinning dependency versions for reproducible builds.Unpinned dependencies may cause documentation build failures when upstream packages release breaking changes. Consider specifying version constraints (e.g.,
sphinx>=7.0,<8.0) to ensure consistent builds across environments.📦 Example with version pins
-sphinx -sphinx-rtd-theme -sphinx-copybutton +sphinx>=7.0,<9.0 +sphinx-rtd-theme>=2.0,<3.0 +sphinx-copybutton>=0.5,<1.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/requirements.txt` around lines 1 - 3, The requirements.txt currently lists unpinned packages (sphinx, sphinx-rtd-theme, sphinx-copybutton); update that file to add stable version constraints (e.g., use >= and < ranges or exact pins) for each package to ensure reproducible builds — replace the bare names "sphinx", "sphinx-rtd-theme", and "sphinx-copybutton" with pinned versions such as "sphinx>=7.0,<8.0" (and appropriate compatible ranges for the other two) so CI and local builds use consistent dependency versions.src/main/scala/components/PC.scala (1)
16-19: Theio.haltinput is now unused.The halt-based gating has been moved to the
npccalculation inCore.scala(lines 178-187), which is a reasonable centralization of control logic. However,io.halt(line 7) is still declared and connected fromCore.scalabut no longer read within this module.Consider removing the unused
haltinput to avoid confusion, or add a comment explaining why it's retained.♻️ Option: Remove unused halt input
class PC extends Module{ val io = IO(new Bundle { val in = Input(SInt(32.W)) - val halt = Input(Bool()) val out = Output(SInt(32.W)) val pc4 = Output(SInt(32.W)) val pc2 = Output(SInt(32.W)) })Note: This would also require removing
pc.io.halt := haltinCore.scala.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/components/PC.scala` around lines 16 - 19, The io.halt port on the PC module is declared and wired from Core but no longer used (pc4/pc2 are computed unconditionally); either remove the io.halt port and its wiring (remove the halt declaration in the PC module and the pc.io.halt := halt assignment in Core) or explicitly document why it is retained (add a short comment next to the io.halt declaration in PC.scala explaining it is kept for API/backwards-compatibility even though NPC now handles halting). Update whichever change you choose consistently: if removing, ensure Core.scala no longer references pc.io.halt; if keeping, add the comment and leave wiring as-is.src/main/scala/components/InstructionFetch.scala (1)
19-26: State machine removal simplifies handshake but leavesio.stallunused.The commented-out state machine was gating request issuance based on stall conditions. With this change,
io.stall(line 10) is now declared but only referenced in commented code (line 35).This simplification aligns with the SRAM cycle-delay fix mentioned in the PR objectives and matches the pattern used in
MemoryFetch(dccmRsp.ready := true.B). However, consider either removing the unusedstallinput or documenting why it's retained for future use.🤖 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 19 - 26, The io.stall signal is now unused after removing the state machine (state_reg, next_state, coreInstrResp.ready logic) so either remove the stall input from the InstructionFetch IO bundle or explicitly document/anchor it for interface stability: delete the unused io.stall declaration and remove the commented state machine remnants (state_reg, next_state, coreInstrResp.ready) to avoid dead signals, or if you must keep it for compatibility, add a clear ScalaDoc/TODO above io.stall and tie it to a harmless default (e.g., ignore or drive false) inside InstructionFetch so it is intentionally retained; reference the symbols io.stall, state_reg, next_state, coreInstrResp.ready and the MemoryFetch pattern (dccmRsp.ready) when making the change.src/main/scala/components/SRamTop.scala (1)
14-22: Clean up commented-out register-based handshake code.The commented-out
validRegandreq_validregister logic spans multiple locations in this file. Since the handshake has been simplified as part of the cycle delay fix, consider removing this dead code to improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/components/SRamTop.scala` around lines 14 - 22, Remove the dead/commented-out handshake/register code: delete the commented declarations and references to validReg and req_valid and the commented when/otherwise block as well as the commented assignment to io.rsp.valid, since the handshake was simplified (io.req.ready := true.B) and these commented lines no longer serve a purpose; ensure no other code references validReg or req_valid remain in SRamTop so compilation and readability are improved.src/main/scala/components/MemoryFetch.scala (2)
45-51: Remove commented-out holding logic or document rationale.This commented-out code for holding
funct3andoffsetvalues during response is dead code. If it was removed intentionally as part of the cycle delay fix, consider removing it entirely to improve readability. If it may be needed later, add a brief comment explaining why it was disabled.🤖 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 45 - 51, The commented-out branch that would hold funct3 and offset when io.dccmRsp.valid is false is dead code and should be removed or documented; either delete the commented block around io.dccmRsp.valid or add a short rationale comment explaining why holding behavior was removed (e.g., cycle-delay fix) so future readers know it was intentional; update references in MemoryFetch to funct3, offset, io.f3 and io.aluResultIn(1,0) accordingly.
125-128: Consider removing unusedamoRdValport.The comment indicates
amoRdValis unused and hardcoded to0.U. If this port serves no purpose, removing it would reduce interface complexity. If it's intended for future use, consider documenting the planned use case.🤖 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 125 - 128, The io.amoRdVal port in MemoryFetch.scala is unused (hardcoded to 0.U); remove the unused port and its assignment (the io.amoRdVal.get := 0.U line) from the MemoryFetch IO definition and body, and update any instantiations/usages of the MemoryFetch module (and its IO bundle) so they no longer reference amoRdVal (or alternatively, if you want to keep it for future use, add a clear TODO comment and make it an Option/Vec only where needed). Also adjust any tests or wiring that referenced io.amoRdVal to prevent compile errors..readthedocs.yaml (1)
21-26: Remove redundant commented-out section.Lines 17-19 already configure Python requirements installation. The commented-out section (lines 24-26) duplicates this configuration and should be removed to avoid confusion.
🧹 Proposed cleanup
python: install: - requirements: docs/requirements.txt - -# Optionally, but recommended, -# declare the Python requirements required to build your documentation -# See https://docs.readthedocs.io/en/stable/guides/reproducible-builds.html -# python: -# install: -# - requirements: docs/requirements.txt -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.readthedocs.yaml around lines 21 - 26, Remove the redundant commented-out Python requirements block that duplicates the existing configuration: delete the commented lines containing the snippet "python: install: - requirements: docs/requirements.txt" (the commented block shown in the diff) so only the primary configured Python requirements remain and the file no longer contains duplicate commented sections.docs/index.rst (1)
4-4: Consider clarifying that ISA extensions are configurable, not always enabled by default.Current wording reads as a fixed profile. A small wording tweak would better match the configuration model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/index.rst` at line 4, The sentence describing NucleusRV's ISA (the line referencing "RV32IMAFC") implies a fixed profile; update the wording to state that ISA extensions (I, M, A, F, C) are configurable and may be enabled per build/config rather than always included by default — edit the sentence mentioning "RV32IMAFC" and/or "32-bit 5-stage pipelined RISC-V core" to clarify that the core supports the RV32 base with optional/configurable I, M, A, F, C extensions so readers understand extensions are selectable via configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/concepts/configuration.rst`:
- Around line 11-21: The Configs example in the docs sets M, F, and Zicsr to
true but the canonical defaults in the code are false; update the Configs case
class example so the Boolean fields M, F, and Zicsr are false to match the
implementation in Configs.scala (i.e., change M : Boolean = true, F : Boolean =
true, Zicsr : Boolean = true to M : Boolean = false, F : Boolean = false, Zicsr
: Boolean = false).
In `@docs/guides/arch-tests.rst`:
- Around line 20-21: The docs currently run pip installs referencing
riscof/riscv-arch-test paths that live in a git submodule; add a step to
initialize submodules before those pip commands (run git submodule update --init
--recursive) or explicitly state that users must initialize the riscof submodule
first so the riscof/riscv-arch-test/riscv-ctg and riscof-isac directories exist.
In `@docs/guides/fpga-integration.rst`:
- Around line 22-23: The documentation references a missing image asset
"/_static/fpga_integration_diagram.png" in the image directive within
docs/guides/fpga-integration.rst; fix this by either adding the actual diagram
file to the repository under the _static assets so the directive points to an
existing file, or remove (or comment out) the image directive line ".. image::
/_static/fpga_integration_diagram.png" from the RST to avoid a broken image
link.
In `@docs/guides/running-assembly.rst`:
- Around line 24-29: Update the GCC invocation to use the documented ISA and the
correct file paths: replace the `-march=rv32gcv` flag with `-march=rv32imafc`,
and reference the assembly and ELF files in the tools directory (e.g., use
`tools/program.S` as the input to riscv32-unknown-elf-gcc and produce
`tools/program.elf`), then pass `tools/program.elf` into `tools/makehex.py`;
adjust the `riscv32-unknown-elf-gcc` command and the `python3 tools/makehex.py`
invocation accordingly so the filenames and ISA flag are consistent with the
rest of the document.
In `@docs/quickstart.rst`:
- Around line 34-37: The quickstart sbt command calls the Scala main object
NRVDriver incorrectly; update the command to use sbt's runMain invocation by
inserting runMain before nucleusrv.components.NRVDriver (i.e., use runMain
nucleusrv.components.NRVDriver ...) so the documentation matches the
repository's Python automation and actually invokes the NRVDriver main.
In `@docs/reference/config-options.rst`:
- Around line 11-21: The documentation's Configs case class defaults are out of
sync with the implementation: change the default values for M, F, and Zicsr from
true to false in the shown Configs snippet (the Configs case class declaration)
so they match the source implementation; also search and update the other
duplicated Configs snippets in this file where M, F, or Zicsr are listed (the
other occurrences reported in the review) to ensure all doc examples reflect the
actual defaults in the Configs class.
In `@src/main/resources/sram.v`:
- Around line 118-126: The SRAM is sampling unregistered inputs (csb0, web0,
addr0, wmask0, din0) on negedge clk0 causing stability issues; update the sram
instance wiring so it uses the registered outputs produced by SRamTop.scala
(csb, addr_o, wdata_o, wmask_o, we_o) instead of the direct combinational inputs
(csb0/web0/addr0/wmask0/din0), or alternatively register those signals inside
SRamTop.scala before they are passed down; locate the write logic block in
sram.v (the if (!csb0 && !web0) ... mem[...] assignments) and replace references
to the combinational signals with the registered signal names (csb, addr_o,
wdata_o, wmask_o, we_o) so the SRAM samples stable, registered values on negedge
clk0.
In `@src/main/scala/components/Core.scala`:
- Around line 407-413: The AMO/store write-data forward currently checks both
rs1 and rs2 (ex_reg_ins(19,15) and ex_reg_ins(24,20)) and can wrongly forward
mem_reg_rd when it only matches rs1; restrict the compare to the store data
source by replacing the dual-register check with a single check against rs2
(ex_reg_ins(24,20)) in the MEM.io.writeData assignment (the expression inside
the Mux that tests (mem_reg_wra === ex_reg_ins(...))); update the condition in
the mem_reg_isAMO && (mem_reg_wra =/= 0.U) && (...) clause so it only compares
mem_reg_wra to ex_reg_ins(24,20) rather than ex_reg_ins(19,15).
- Around line 293-297: The AMO ALU is being driven with EX/MEM pipeline
registers (ex_reg_wd, ex_reg_amoOp) causing the first AMO cycle to use stale
data; change the assignments that set EX.amo_memData, EX.amo_src2, and
EX.amo_op_code to source the current ID/EX-stage values instead of
ex_reg_wd/ex_reg_amoOp so the execute-stage instruction's rs2 and opcode are
used. Locate the code that assigns EX.amo_memData.get := amo_old_value.get,
EX.amo_src2.get := ex_reg_wd and EX.amo_op_code.get := ex_reg_amoOp and replace
those right-hand-side signals with the corresponding ID/EX pipeline signals (the
ID-stage wd and amoOp signals in your design) so the ALU sees the instruction
currently in EX rather than the previous one.
- Around line 567-569: The expression using the unary ! on a UInt is incorrect:
replace the Boolean comparison `!mem_reg_ctl_memToReg === 1.U` with a proper
inequality on the UInt `mem_reg_ctl_memToReg =/= 1.U` in the instruction_retired
assignment (inside the Zicsr block where instruction_retired is computed from
mem_reg_ins, ID.ifid_flush, MEM.io.stall, io.stall, mem_reg_ctl_memToReg and
io.dmemRsp.valid) and ensure the updated value is still assigned to
ID.csr_i_instr_retired.get as before.
- Around line 453-472: The problem is that sc_issued and sc_matched are set and
then cleared in the same cycle because independent when blocks allow the clear
to override the set; update the logic so the clear is gated behind the inverse
of the set condition (or make the set/clear a single when/elsewhen chain) —
e.g., convert the separate assignments for sc_issued (currently using
ex_reg_isSC.get, MEM.io.stall, EX.stall) into a single when/elsewhen that sets
sc_issued.get := true.B when ex_reg_isSC.get && !MEM.io.stall and only clears it
when !MEM.io.stall && !EX.stall && !ex_reg_isSC.get (or use elsewhen to ensure
the set wins), and do the same for sc_matched (use a combined when/elsewhen that
sets sc_matched.get when ex_reg_isSC.get && reservationFile.get.matchAddr and
only clears it when !MEM.io.stall && !ex_reg_isSC.get).
In `@src/main/scala/components/Execute.scala`:
- Around line 215-216: The f_in_valid is being cleared as soon as
fpu.div_sqrt_valid_out goes high, which allows the same multi-cycle instruction
to be reissued if id_ex_ins is still held; change the logic so f_in_valid is
only cleared when the EX-stage actually accepts/advances the instruction (e.g.,
tie the clear to the EX-stage accept/advance handshake or the id_ex_ins update
signal rather than div_sqrt_valid_out alone). Specifically, update the code that
sets/clears f_in_valid (and the related f_stall/f_multi_cycle_inst handling) to
sample and clear on an EX accept/advance signal (or id_ex acceptance/ready) so a
held id_ex_ins cannot cause a second div_sqrt_valid pulse; apply the same fix
pattern to the similar block around lines 254-261.
In `@src/main/scala/components/HazardUnit.scala`:
- Around line 137-141: The current override clears amo_stall when only addr_EX
=/= addr_MEM, which can drop a real hazard for the AMO in ID; update the
condition in the isAMO_EX && isAMO_MEM branch to also compare against addr_ID so
we only clear amo_stall when neither EX nor MEM addresses match the ID-stage
AMO. Concretely, modify the inner when to require addr_EX =/= addr_MEM &&
addr_EX =/= addr_ID && addr_MEM =/= addr_ID (or equivalently check that addr_ID
differs from both addr_EX and addr_MEM) before setting amo_stall := false.B so
hazards originating from addr_ID are not masked.
- Around line 45-67: Restore load-use hazard detection in HazardUnit: re-enable
the check using id_ex_memRead, id_ex_rd, id_rs1 and id_rs2 (the commented when
block) so that if a prior-stage load will write the same register as an ID-stage
operand you assert standard_stall and set io.ctl_mux := false.B, io.pc_write :=
false.B, io.if_reg_write := false.B, io.take_branch := false.B to insert a
bubble; ensure the condition still guards rd != 0 and rs checks (use the
original logical expression) and avoid stalling on true forwarded values like
ALU addresses; alternatively, if the ISA intentionally removed load latency,
remove id_ex_memRead/id_ex_rd/id_rs1/id_rs2 from the interface and callers
instead of leaving unused signals.
In `@src/main/scala/components/InstructionDecode.scala`:
- Around line 230-234: The current shared writeData MuxCase wrongly selects
CSR-forwarded data once using a condition that ORs registerRs1 and registerRs2,
then reuses writeData for rs1/rs2/rs3 bypasses; change this so each source
operand has its own forwarded-value computation (e.g., writeDataRs1,
writeDataRs2, writeDataRs3) that checks only that operand against
id_ex_rd/ex_mem_rd/writeReg and uses the corresponding CSR data signals
(io.csr_Ex_data, io.csr_Mem_data, io.csr_Wb_data) and control flags (io.csr_Ex,
io.csr_Mem, io.csr_Wb, io.id_ex_regWr, io.ex_mem_regWr, io.ctl_writeEnable(0))
so a CSR hit on rs1 cannot pollute rs2/rs3.
- Around line 293-301: The AMO forwarding path currently overwrites both
operands (io.readData1 and io.readData2); change the forwarding in the AMO
hazard branches so only io.readData1 is assigned from the forwarded value (use
io.ex_result for hdu.io.operandForwardEX.get and io.ex_mem_result for
hdu.io.operandForwardMEM.get), leaving io.readData2 untouched; apply the same
fix to the other AMO forwarding occurrence that mirrors these conditions (the
block that also checks hdu.io.operandForwardEX/operandForwardMEM and currently
writes both readData1/readData2), and ensure hdu.io.addr_id remains sourced from
the correct (possibly forwarded) readData1 only.
In `@src/main/scala/components/SRamTop.scala`:
- Around line 47-55: The response valid is being asserted combinationally while
sram.io.rdata_o is registered and available next cycle; add a 1-cycle register
(e.g., rspValidReg) that captures the read-request condition (when io.req.valid
&& !io.req.bits.isWrite) and drive io.rsp.valid := rspValidReg so valid aligns
with data latency, and remove the duplicate assignment to rdata by assigning
sram.io.rdata_o to your single rdata register only once (use the registered
rdata output to update rdata on the next clock edge); update the read-path
handling in the same when block (referencing io.req.valid, io.req.bits.isWrite,
sram.io.rdata_o, rdata and io.rsp.valid) to use the registered valid/data
handshake.
---
Outside diff comments:
In `@src/main/scala/components/Configs.scala`:
- Around line 3-13: Default ISA behavior changed because Configs default enables
A and TRACE and disables M, F, Zicsr (so default is RV32IA with A true); update
the project documentation/changelog and/or adjust instantiations that rely on
implicit Configs() to explicitly pass desired flags. Specifically, add a
migration note describing the new defaults for Configs (symbols: Configs, XLEN,
M, F, Zicsr, A, TRACE) and either change callers that rely on implicit defaults
(e.g., where Top constructs a Configs() without args) to pass explicit
parameters or document that users must now instantiate Configs(M = true, F =
true, Zicsr = true, ...) when expecting previous behavior.
---
Minor comments:
In `@docs/guides/running-assembly.rst`:
- Around line 9-10: The doc references program.S in the tools/ directory but
subsequent commands operate on program.S and program.elf in the current
directory, which will break copy/paste; update the guide so the paths are
consistent (either change the example files to tools/program.S and
tools/program.elf in the commands or move the initial sentence to indicate the
file is in the repo root), and adjust all occurrences of "program.S" and
"program.elf" (including the commands around lines 9–10 and 28–29) to use the
same directory naming (e.g., tools/program.S and tools/program.elf) so the
instructions work when run verbatim.
In `@docs/installation.rst`:
- Around line 31-34: The docs currently recommend building the toolchain with
--with-arch=rv32gc which includes M/F/D/C but the default Configs.scala produces
an RV32IA core; update the installation instructions to either change the
configure flag to --with-arch=rv32ia to match the default Configs.scala, or keep
--with-arch=rv32gc but add a clear note that rv32gc is a superset and users must
compile with appropriate -march flags (or choose a matching --with-arch like
rv32imfc) for their enabled extensions; reference Configs.scala in the note so
readers know where the default core is defined and show the suggested short note
text advising matching toolchain arch to enabled extensions.
In `@docs/reference/csr-map.rst`:
- Line 60: Replace the inconsistent CSR name "mcountinh" with the canonical
"mcountinhibit" in the docs: update the table row that currently shows "| 0x320
| mcountinh | Machine counter-inhibit register |" and any other occurrences
(including the heading around lines 100-101) so the CSR name is consistently
"mcountinhibit" throughout docs/reference/csr-map.rst; ensure cross-references
and anchors (if any) use the same term.
In `@docs/reference/pipeline-stages.rst`:
- Line 61: The page mixes two names for the debug port (io.pin and io_pin); pick
one convention and make it consistent across the page (or explicitly state a
mapping between Chisel and Verilog names). Update every occurrence (e.g., the
instances currently showing io.pin at line ~61 and io_pin at line ~73) to the
chosen form or add a short note like “Chisel: io.pin ↔ Verilog: io_pin” and
apply that consistently so readers aren’t confused.
---
Nitpick comments:
In @.readthedocs.yaml:
- Around line 21-26: Remove the redundant commented-out Python requirements
block that duplicates the existing configuration: delete the commented lines
containing the snippet "python: install: - requirements: docs/requirements.txt"
(the commented block shown in the diff) so only the primary configured Python
requirements remain and the file no longer contains duplicate commented
sections.
In `@docs/index.rst`:
- Line 4: The sentence describing NucleusRV's ISA (the line referencing
"RV32IMAFC") implies a fixed profile; update the wording to state that ISA
extensions (I, M, A, F, C) are configurable and may be enabled per build/config
rather than always included by default — edit the sentence mentioning
"RV32IMAFC" and/or "32-bit 5-stage pipelined RISC-V core" to clarify that the
core supports the RV32 base with optional/configurable I, M, A, F, C extensions
so readers understand extensions are selectable via configuration.
In `@docs/installation.rst`:
- Around line 49-51: Replace the placeholder sbt installation comment with
concrete instructions and a reference link: add the common Debian/Ubuntu
installation commands for sbt (install apt-transport-https, add the sbt
repository GPG key and repository, apt-get update, apt-get install sbt) and
include a direct link to the official sbt installation page so users on other
distros can follow vendor guidance; update the docs/installation.rst section
that currently contains the "# Install sbt via official repositories" comment to
include these commands and the URL.
In `@docs/requirements.txt`:
- Around line 1-3: The requirements.txt currently lists unpinned packages
(sphinx, sphinx-rtd-theme, sphinx-copybutton); update that file to add stable
version constraints (e.g., use >= and < ranges or exact pins) for each package
to ensure reproducible builds — replace the bare names "sphinx",
"sphinx-rtd-theme", and "sphinx-copybutton" with pinned versions such as
"sphinx>=7.0,<8.0" (and appropriate compatible ranges for the other two) so CI
and local builds use consistent dependency versions.
In `@riscof/nucleusrv/riscof_nucleusrv.py`:
- Around line 174-198: The command chains built into simcmd (in both branches
where simcmd is assembled) use ';' which allows later steps to run even if
earlier steps like objcopy, hexdump, or objdump fail; change the separators
between those commands to '&&' so the chain fails fast and downstream steps
(including the sbt/verilator/run steps) won't execute on earlier errors—update
the constructions that call self.objcopy, self.hexdump, self.objdump and the
branch that calls self.sbt/verilator to join commands with '&&' instead of ';'.
In `@src/main/resources/sram.v`:
- Around line 65-87: The file contains a large commented-out legacy register
path (csb0_reg, web0_reg, wmask0_reg, addr0_reg, din0_reg, dout0 and the clk0
always block plus debug prints) that clutters the active design; remove this
dead block or replace it with a single-line note pointing to the
rationale/history (or a changelog entry) so the module only contains the current
implementation and not inline legacy code. Ensure any needed debug behavior is
documented elsewhere and no references to these symbols remain in active code.
- Around line 118-126: The byte-lane write logic is hard-coded for 4
masks/32-bit and must be parameterized: inside the same conditional (when !csb0
&& !web0) replace the four explicit wmask0[x] checks with a loop over i from 0
to NUM_WMASKS-1 that checks wmask0[i] and assigns the corresponding slice of
mem[addr0] from bit index high = (i+1)*(DATA_WIDTH/NUM_WMASKS)-1 down to low =
i*(DATA_WIDTH/NUM_WMASKS) with din0[high:low]; ensure you use the NUM_WMASKS and
DATA_WIDTH parameters to compute slice widths and that the loop variable and
slicing match the net names (wmask0, mem, addr0, din0) so the code works for
non-32-bit configurations.
In `@src/main/scala/components/InstructionFetch.scala`:
- Around line 19-26: The io.stall signal is now unused after removing the state
machine (state_reg, next_state, coreInstrResp.ready logic) so either remove the
stall input from the InstructionFetch IO bundle or explicitly document/anchor it
for interface stability: delete the unused io.stall declaration and remove the
commented state machine remnants (state_reg, next_state, coreInstrResp.ready) to
avoid dead signals, or if you must keep it for compatibility, add a clear
ScalaDoc/TODO above io.stall and tie it to a harmless default (e.g., ignore or
drive false) inside InstructionFetch so it is intentionally retained; reference
the symbols io.stall, state_reg, next_state, coreInstrResp.ready and the
MemoryFetch pattern (dccmRsp.ready) when making the change.
In `@src/main/scala/components/MemoryFetch.scala`:
- Around line 45-51: The commented-out branch that would hold funct3 and offset
when io.dccmRsp.valid is false is dead code and should be removed or documented;
either delete the commented block around io.dccmRsp.valid or add a short
rationale comment explaining why holding behavior was removed (e.g., cycle-delay
fix) so future readers know it was intentional; update references in MemoryFetch
to funct3, offset, io.f3 and io.aluResultIn(1,0) accordingly.
- Around line 125-128: The io.amoRdVal port in MemoryFetch.scala is unused
(hardcoded to 0.U); remove the unused port and its assignment (the
io.amoRdVal.get := 0.U line) from the MemoryFetch IO definition and body, and
update any instantiations/usages of the MemoryFetch module (and its IO bundle)
so they no longer reference amoRdVal (or alternatively, if you want to keep it
for future use, add a clear TODO comment and make it an Option/Vec only where
needed). Also adjust any tests or wiring that referenced io.amoRdVal to prevent
compile errors.
In `@src/main/scala/components/PC.scala`:
- Around line 16-19: The io.halt port on the PC module is declared and wired
from Core but no longer used (pc4/pc2 are computed unconditionally); either
remove the io.halt port and its wiring (remove the halt declaration in the PC
module and the pc.io.halt := halt assignment in Core) or explicitly document why
it is retained (add a short comment next to the io.halt declaration in PC.scala
explaining it is kept for API/backwards-compatibility even though NPC now
handles halting). Update whichever change you choose consistently: if removing,
ensure Core.scala no longer references pc.io.halt; if keeping, add the comment
and leave wiring as-is.
In `@src/main/scala/components/SRamTop.scala`:
- Around line 14-22: Remove the dead/commented-out handshake/register code:
delete the commented declarations and references to validReg and req_valid and
the commented when/otherwise block as well as the commented assignment to
io.rsp.valid, since the handshake was simplified (io.req.ready := true.B) and
these commented lines no longer serve a purpose; ensure no other code references
validReg or req_valid remain in SRamTop so compilation and readability are
improved.
🪄 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: 7f3c1856-1119-464e-a84b-c51404a3f4b0
⛔ Files ignored due to path filters (1)
docs/03_dev/images/if_module.svgis excluded by!**/*.svg
📒 Files selected for processing (40)
.gitignore.readthedocs.yamldocs/01_overview/index.rstdocs/02_user/index.rstdocs/03_dev/index.rstdocs/03_dev/instruction_fetch.rstdocs/Makefiledocs/concepts/configuration.rstdocs/concepts/isa-support.rstdocs/concepts/memory-interface.rstdocs/concepts/pipeline.rstdocs/conf.pydocs/guides/arch-tests.rstdocs/guides/building-c-programs.rstdocs/guides/fpga-integration.rstdocs/guides/running-assembly.rstdocs/guides/waveform-simulation.rstdocs/index.rstdocs/installation.rstdocs/introduction.rstdocs/quickstart.rstdocs/reference/config-options.rstdocs/reference/csr-map.rstdocs/reference/pipeline-stages.rstdocs/requirements.txtriscof/nucleusrv/nucleusrv_isa.yamlriscof/nucleusrv/riscof_nucleusrv.pysrc/main/resources/sram.vsrc/main/scala/components/CSR.scalasrc/main/scala/components/Configs.scalasrc/main/scala/components/Core.scalasrc/main/scala/components/Execute.scalasrc/main/scala/components/HazardUnit.scalasrc/main/scala/components/InstructionDecode.scalasrc/main/scala/components/InstructionFetch.scalasrc/main/scala/components/MemoryFetch.scalasrc/main/scala/components/PC.scalasrc/main/scala/components/SRamTop.scalasrc/main/scala/csr/CSRBundles.scalasrc/main/scala/csr/CSRRegFile.scala
💤 Files with no reviewable changes (4)
- docs/02_user/index.rst
- docs/03_dev/index.rst
- docs/01_overview/index.rst
- docs/03_dev/instruction_fetch.rst
Comment on lines
+11
to
+21
| case class Configs( | ||
| XLEN : Int = 32, | ||
| M : Boolean = true, | ||
| F : Boolean = true, | ||
| C : Boolean = false, | ||
| A : Boolean = true, | ||
| Zicsr : Boolean = true, | ||
| TRACE : Boolean = true, | ||
| HARTID : Int = 1, | ||
| ARCHID : Int = 1 | ||
| ) |
There was a problem hiding this comment.
Configs example shows incorrect defaults for M, F, and Zicsr.
This snippet should match src/main/scala/components/Configs.scala defaults (false for M, F, Zicsr) to prevent incorrect baseline assumptions.
🛠️ Proposed doc fix
case class Configs(
XLEN : Int = 32,
- M : Boolean = true,
- F : Boolean = true,
+ M : Boolean = false,
+ F : Boolean = false,
C : Boolean = false,
A : Boolean = true,
- Zicsr : Boolean = true,
+ Zicsr : Boolean = false,
TRACE : Boolean = true,
HARTID : Int = 1,
ARCHID : Int = 1
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case class Configs( | |
| XLEN : Int = 32, | |
| M : Boolean = true, | |
| F : Boolean = true, | |
| C : Boolean = false, | |
| A : Boolean = true, | |
| Zicsr : Boolean = true, | |
| TRACE : Boolean = true, | |
| HARTID : Int = 1, | |
| ARCHID : Int = 1 | |
| ) | |
| case class Configs( | |
| XLEN : Int = 32, | |
| M : Boolean = false, | |
| F : Boolean = false, | |
| C : Boolean = false, | |
| A : Boolean = true, | |
| Zicsr : Boolean = false, | |
| TRACE : Boolean = true, | |
| HARTID : Int = 1, | |
| ARCHID : Int = 1 | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/concepts/configuration.rst` around lines 11 - 21, The Configs example in
the docs sets M, F, and Zicsr to true but the canonical defaults in the code are
false; update the Configs case class example so the Boolean fields M, F, and
Zicsr are false to match the implementation in Configs.scala (i.e., change M :
Boolean = true, F : Boolean = true, Zicsr : Boolean = true to M : Boolean =
false, F : Boolean = false, Zicsr : Boolean = false).
Comment on lines
+20
to
+21
| pip3 install git+https://github.com/riscv/riscof.git | ||
| pip3 install -e riscof/riscv-arch-test/riscv-ctg riscof/riscv-arch-test/riscv-isac |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the riscof plugin directories exist
echo "Checking riscof directory structure:"
fd -t d "riscv-ctg|riscv-isac" riscof/ 2>/dev/null || echo "Directories not found"
tree -L 3 riscof/ 2>/dev/null | head -30Repository: merledu/nucleusrv
Length of output: 437
🏁 Script executed:
#!/bin/bash
# Check riscov-arch-test contents and submodule status
echo "=== Contents of riscof/riscv-arch-test/ ==="
ls -la riscof/riscv-arch-test/ 2>&1 || echo "Directory listing failed"
echo ""
echo "=== Checking for riscv-ctg and riscv-isac ==="
ls -ld riscof/riscv-arch-test/riscv-ctg 2>&1
ls -ld riscof/riscv-arch-test/riscv-isac 2>&1
echo ""
echo "=== Checking .gitmodules ==="
cat .gitmodules 2>&1 || echo "No .gitmodules file"Repository: merledu/nucleusrv
Length of output: 697
Add submodule initialization to setup instructions.
The pip install command on lines 20-21 references riscof/riscv-arch-test/riscv-ctg and riscof/riscv-arch-test/riscv-isac, but these directories do not exist. The riscof/riscv-arch-test is a git submodule that must be initialized first. Add a step to initialize submodules before the pip install commands:
git submodule update --init --recursive
Alternatively, clarify in the documentation that users must initialize git submodules as part of the setup process.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/guides/arch-tests.rst` around lines 20 - 21, The docs currently run pip
installs referencing riscof/riscv-arch-test paths that live in a git submodule;
add a step to initialize submodules before those pip commands (run git submodule
update --init --recursive) or explicitly state that users must initialize the
riscof submodule first so the riscof/riscv-arch-test/riscv-ctg and riscof-isac
directories exist.
Comment on lines
+22
to
+23
| .. image:: /_static/fpga_integration_diagram.png | ||
| :alt: FPGA Integration Diagram |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the FPGA integration diagram exists
fd -t f "fpga_integration_diagram.png" docs/Repository: merledu/nucleusrv
Length of output: 43
🏁 Script executed:
# Search for the image file across the entire repository
find . -name "*fpga_integration_diagram*" -type f 2>/dev/null
# Also check if any _static directories exist
find . -type d -name "_static" 2>/dev/nullRepository: merledu/nucleusrv
Length of output: 43
🏁 Script executed:
# Check docs directory structure to understand how static assets are handled
ls -la docs/ | head -20
# Check if there are any image files in the docs directory
find docs/ -type f \( -name "*.png" -o -name "*.jpg" -o -name "*.svg" \) 2>/dev/nullRepository: merledu/nucleusrv
Length of output: 794
Add the missing FPGA integration diagram asset.
The guide references /_static/fpga_integration_diagram.png, but this file does not exist in the repository. Either include the diagram in the PR or remove the image directive from the documentation to prevent broken image links.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/guides/fpga-integration.rst` around lines 22 - 23, The documentation
references a missing image asset "/_static/fpga_integration_diagram.png" in the
image directive within docs/guides/fpga-integration.rst; fix this by either
adding the actual diagram file to the repository under the _static assets so the
directive points to an existing file, or remove (or comment out) the image
directive line ".. image:: /_static/fpga_integration_diagram.png" from the RST
to avoid a broken image link.
Comment on lines
+24
to
+29
| Use the RISC-V GCC toolchain to assemble and link your program: | ||
|
|
||
| .. code-block:: bash | ||
|
|
||
| riscv32-unknown-elf-gcc -march=rv32gcv -mabi=ilp32 -static -nostdlib -nostartfiles -T tools/link.ld program.S -o program.elf | ||
| python3 tools/makehex.py program.elf 2048 > imem.hex |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the guide file exists and read the relevant section
find . -name "running-assembly.rst" -type fRepository: merledu/nucleusrv
Length of output: 96
🏁 Script executed:
# Read the full content of running-assembly.rst to see lines around 24-29
cat -n docs/guides/running-assembly.rst | head -50Repository: merledu/nucleusrv
Length of output: 1449
🏁 Script executed:
# Check introduction.rst for ISA description
cat -n docs/introduction.rst | head -50Repository: merledu/nucleusrv
Length of output: 1917
🏁 Script executed:
# Check isa-support.rst for ISA description
find . -name "isa-support.rst" -type fRepository: merledu/nucleusrv
Length of output: 93
🏁 Script executed:
# Search for other -march and -mabi references in documentation
rg "\-march|\-mabi" docs/Repository: merledu/nucleusrv
Length of output: 222
🏁 Script executed:
# Read the isa-support.rst file
cat -n docs/concepts/isa-support.rstRepository: merledu/nucleusrv
Length of output: 1886
🏁 Script executed:
# Check if there are any other references to march/mabi in the codebase
rg "rv32" --type rst --type md docs/Repository: merledu/nucleusrv
Length of output: 315
🏁 Script executed:
# Verify the Zicsr extension mention in isa-support
rg "Zicsr" docs/Repository: merledu/nucleusrv
Length of output: 627
🏁 Script executed:
# Check for more context on rv32gc vs rv32gcv in documentation
rg "rv32gc" docs/Repository: merledu/nucleusrv
Length of output: 315
🏁 Script executed:
# Check the configuration to see what's actually enabled by default
cat -n docs/concepts/configuration.rst | head -100Repository: merledu/nucleusrv
Length of output: 2013
🏁 Script executed:
# Search for any D (double precision) mentions to confirm it's not supported
rg -i "double.precision|RV32IMAFD" docs/Repository: merledu/nucleusrv
Length of output: 43
🏁 Script executed:
# Check GCC RISC-V toolchain documentation for correct march syntax with Zicsr
# Let's search for any build scripts or makefiles that might show correct usage
find . -name "Makefile*" -o -name "*.sh" | head -20Repository: merledu/nucleusrv
Length of output: 1548
🏁 Script executed:
# Check if there's any reference to toolchain configuration or build process
rg "march|mabi" . --type scala --type py --type sh | head -20Repository: merledu/nucleusrv
Length of output: 1495
🏁 Script executed:
# Verify the default configuration more carefully
cat -n docs/concepts/configuration.rst | grep -A 30 "case class Configs"Repository: merledu/nucleusrv
Length of output: 1248
🏁 Script executed:
# Let's check the riscof configuration to see how they compile for NucleusRV
cat -n riscof/nucleusrv/riscof_nucleusrv.py | head -50Repository: merledu/nucleusrv
Length of output: 2189
🏁 Script executed:
# Also check any existing build or test scripts that compile for NucleusRV
find . -name "*.sh" -type f -exec grep -l "riscv32-unknown-elf-gcc" {} \;Repository: merledu/nucleusrv
Length of output: 43
🏁 Script executed:
# Let's verify what the correct RISC-V GCC march string should be with Zicsr
# Check if there's any reference to correct compilation flags in the repo
rg "rv32imaf|rv32imafc" . --type rst --type py --type scalaRepository: merledu/nucleusrv
Length of output: 43
🏁 Script executed:
# Check if the tools/ directory is mentioned elsewhere in context of program.S
rg "tools.*program\.S|program\.S.*tools" docs/Repository: merledu/nucleusrv
Length of output: 330
Fix -march flag and file path in compilation command.
The example uses -march=rv32gcv, which enables the Vector (V) extension that NucleusRV does not support. The documented ISA is RV32IMAFC. Additionally, the command references program.S and program.elf from the current directory, but line 9 explicitly states the assembly file should be created in the tools/ directory—this inconsistency will cause the command to fail.
Suggested fix
- riscv32-unknown-elf-gcc -march=rv32gcv -mabi=ilp32 -static -nostdlib -nostartfiles -T tools/link.ld program.S -o program.elf
+ riscv32-unknown-elf-gcc -march=rv32gc_zicsr -mabi=ilp32f -static -nostdlib -nostartfiles -T tools/link.ld tools/program.S -o tools/program.elf🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/guides/running-assembly.rst` around lines 24 - 29, Update the GCC
invocation to use the documented ISA and the correct file paths: replace the
`-march=rv32gcv` flag with `-march=rv32imafc`, and reference the assembly and
ELF files in the tools directory (e.g., use `tools/program.S` as the input to
riscv32-unknown-elf-gcc and produce `tools/program.elf`), then pass
`tools/program.elf` into `tools/makehex.py`; adjust the
`riscv32-unknown-elf-gcc` command and the `python3 tools/makehex.py` invocation
accordingly so the filenames and ISA flag are consistent with the rest of the
document.
Comment on lines
+34
to
+37
| sbt "nucleusrv.components.NRVDriver \ | ||
| --imem tools/out/program.hex \ | ||
| --dmem tools/out/program.hex \ | ||
| --target-dir generated" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking sbt files for aliases/tasks related to NRVDriver..."
fd -H -I -e sbt | xargs -r rg -n "addCommandAlias|NRVDriver|runMain|mainClass|inputKey|commandKey"
echo "Checking Scala sources for NRVDriver main object..."
fd -H -I -e scala | xargs -r rg -n "object\\s+NRVDriver|def\\s+main\\s*\\("Repository: merledu/nucleusrv
Length of output: 124
🏁 Script executed:
# Check repository structure and find key files
find . -name "build.sbt" -o -name "*.scala" -path "*/components/*" 2>/dev/null | head -20
# Search for NRVDriver anywhere in the repo
rg -i "NRVDriver" --max-count 20
# Check if quickstart.rst exists and show context
find . -name "quickstart.rst" 2>/dev/nullRepository: merledu/nucleusrv
Length of output: 1845
🏁 Script executed:
# Get directory structure to understand project layout
ls -la
fd --type f --type d | head -30Repository: merledu/nucleusrv
Length of output: 2204
🏁 Script executed:
# Check build.sbt for custom aliases or task definitions related to NRVDriver
cat -n build.sbtRepository: merledu/nucleusrv
Length of output: 2713
🏁 Script executed:
# Check the actual quickstart.rst content around lines 34-37
sed -n '30,45p' docs/quickstart.rst
# Also check README.md command
rg -n "nucleusrv.components.NRVDriver" README.md -B2 -A2Repository: merledu/nucleusrv
Length of output: 580
Use runMain for NRVDriver invocation in the quickstart command.
The command on line 34 is missing runMain, which is required by sbt to invoke the NRVDriver Scala main object. The Python automation scripts in the repository correctly use runMain for this invocation; the documentation should match.
🛠️ Proposed doc fix
- sbt "nucleusrv.components.NRVDriver \
+ sbt "runMain nucleusrv.components.NRVDriver \
--imem tools/out/program.hex \
--dmem tools/out/program.hex \
--target-dir generated"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sbt "nucleusrv.components.NRVDriver \ | |
| --imem tools/out/program.hex \ | |
| --dmem tools/out/program.hex \ | |
| --target-dir generated" | |
| sbt "runMain nucleusrv.components.NRVDriver \ | |
| --imem tools/out/program.hex \ | |
| --dmem tools/out/program.hex \ | |
| --target-dir generated" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/quickstart.rst` around lines 34 - 37, The quickstart sbt command calls
the Scala main object NRVDriver incorrectly; update the command to use sbt's
runMain invocation by inserting runMain before nucleusrv.components.NRVDriver
(i.e., use runMain nucleusrv.components.NRVDriver ...) so the documentation
matches the repository's Python automation and actually invokes the NRVDriver
main.
Comment on lines
45
to
+67
| // Standard Load-Use Hazard Detection | ||
| val standard_stall = WireDefault(false.B) | ||
| when ( | ||
| (io.id_ex_memRead || io.branch) | ||
| && ((io.id_ex_rd === io.id_rs1) || (io.id_ex_rd === io.id_rs2)) | ||
| && ( | ||
| ((io.id_ex_rd =/= 0.U) && (io.id_rs1 =/= 0.U)) | ||
| || ((io.id_ex_rd =/= 0.U) && (io.id_rs2 =/= 0.U)) | ||
| ) && !io.id_ex_branch | ||
| ) { | ||
| } | ||
|
|
||
| when ( | ||
| (io.ex_mem_memRead || io.branch) | ||
| && ((io.ex_mem_rd === io.id_rs1) || (io.ex_mem_rd === io.id_rs2)) | ||
| ) { | ||
| when (io.stall) { | ||
| io.ctl_mux := false.B | ||
| io.pc_write := false.B | ||
| io.if_reg_write := false.B | ||
| io.take_branch := false.B | ||
| } | ||
| //when ( | ||
| // (io.id_ex_memRead || io.branch) | ||
| // && ((io.id_ex_rd === io.id_rs1) || (io.id_ex_rd === io.id_rs2)) | ||
| // && ( | ||
| // ((io.id_ex_rd =/= 0.U) && (io.id_rs1 =/= 0.U)) | ||
| // || ((io.id_ex_rd =/= 0.U) && (io.id_rs2 =/= 0.U)) | ||
| // ) && !io.id_ex_branch | ||
| //) { | ||
| //} | ||
|
|
||
| //when ( | ||
| // (io.ex_mem_memRead || io.branch) | ||
| // && ((io.ex_mem_rd === io.id_rs1) || (io.ex_mem_rd === io.id_rs2)) | ||
| //) { | ||
| //} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.scala" | head -20Repository: merledu/nucleusrv
Length of output: 951
🏁 Script executed:
git ls-files | grep -E "\.(scala|java)$" | head -30Repository: merledu/nucleusrv
Length of output: 1313
🏁 Script executed:
rg "id_ex_rd|id_rs1|id_rs2" --type scala -A 2 -B 2Repository: merledu/nucleusrv
Length of output: 5139
🏁 Script executed:
rg "ex_stall|mem_stall" --type scala -A 3 -B 3Repository: merledu/nucleusrv
Length of output: 3247
🏁 Script executed:
cat -n src/main/scala/components/HazardUnit.scalaRepository: merledu/nucleusrv
Length of output: 5740
🏁 Script executed:
cat -n src/main/scala/components/Execute.scala | head -100Repository: merledu/nucleusrv
Length of output: 3889
🏁 Script executed:
rg "stall" src/main/scala/components/Execute.scala -B 2 -A 2Repository: merledu/nucleusrv
Length of output: 1280
🏁 Script executed:
cat -n src/main/scala/components/MemoryFetch.scala | head -100Repository: merledu/nucleusrv
Length of output: 4121
🏁 Script executed:
rg "stall" src/main/scala/components/MemoryFetch.scala -B 2 -A 2Repository: merledu/nucleusrv
Length of output: 1153
🏁 Script executed:
rg "id_ex_memRead|memRead|rd.*rs" src/main/scala/components/Execute.scala src/main/scala/components/MemoryFetch.scala -B 1 -A 1Repository: merledu/nucleusrv
Length of output: 43
🏁 Script executed:
cat -n src/main/scala/components/ForwardingUnit.scalaRepository: merledu/nucleusrv
Length of output: 2913
🏁 Script executed:
rg "load|Load|memRead" src/main/scala/components/ForwardingUnit.scala -B 2 -A 2Repository: merledu/nucleusrv
Length of output: 43
Load-use hazard detection is missing and not reconstructed elsewhere.
The commented code (lines 53–67) checked for load-use hazards: (io.id_ex_memRead || io.branch) && ((io.id_ex_rd === io.id_rs1) || (io.id_ex_rd === io.id_rs2)). This check is now disabled, and no equivalent detection exists in Execute.scala (which only stalls for MDU/FPU operations) or MemoryFetch.scala (which returns false.B for stall). The ForwardingUnit cannot compensate because it has no knowledge of id_ex_memRead and would incorrectly forward the ALU result (memory address) as the operand value instead of the actual loaded data.
Any instruction sequence with a load followed by immediate use of the loaded register will compute with wrong values. The inputs id_ex_rd, id_rs1, id_rs2, and id_ex_memRead are now unused and should be either restored to hazard detection or removed from the interface if the architecture has been changed to tolerate load latency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/scala/components/HazardUnit.scala` around lines 45 - 67, Restore
load-use hazard detection in HazardUnit: re-enable the check using
id_ex_memRead, id_ex_rd, id_rs1 and id_rs2 (the commented when block) so that if
a prior-stage load will write the same register as an ID-stage operand you
assert standard_stall and set io.ctl_mux := false.B, io.pc_write := false.B,
io.if_reg_write := false.B, io.take_branch := false.B to insert a bubble; ensure
the condition still guards rd != 0 and rs checks (use the original logical
expression) and avoid stalling on true forwarded values like ALU addresses;
alternatively, if the ISA intentionally removed load latency, remove
id_ex_memRead/id_ex_rd/id_rs1/id_rs2 from the interface and callers instead of
leaving unused signals.
Comment on lines
+137
to
+141
| // Consecutive AMOs optimization | ||
| when(isAMO_EX && isAMO_MEM) { | ||
| when(addr_EX =/= addr_MEM) { | ||
| amo_stall := false.B // Allow different AMO addresses | ||
| } |
There was a problem hiding this comment.
Don't clear amo_stall based only on addr_EX =/= addr_MEM.
This override ignores addr_ID, so it can drop a real hazard for the current ID-stage AMO and mask the earlier decision from Lines 116-135.
Suggested fix
- when(isAMO_EX && isAMO_MEM) {
- when(addr_EX =/= addr_MEM) {
- amo_stall := false.B // Allow different AMO addresses
- }
- }
+ when(isAMO_EX && isAMO_MEM &&
+ (addr_ID =/= addr_EX) && (addr_ID =/= addr_MEM)) {
+ amo_stall := false.B
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Consecutive AMOs optimization | |
| when(isAMO_EX && isAMO_MEM) { | |
| when(addr_EX =/= addr_MEM) { | |
| amo_stall := false.B // Allow different AMO addresses | |
| } | |
| when(isAMO_EX && isAMO_MEM && | |
| (addr_ID =/= addr_EX) && (addr_ID =/= addr_MEM)) { | |
| amo_stall := false.B | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/scala/components/HazardUnit.scala` around lines 137 - 141, The
current override clears amo_stall when only addr_EX =/= addr_MEM, which can drop
a real hazard for the AMO in ID; update the condition in the isAMO_EX &&
isAMO_MEM branch to also compare against addr_ID so we only clear amo_stall when
neither EX nor MEM addresses match the ID-stage AMO. Concretely, modify the
inner when to require addr_EX =/= addr_MEM && addr_EX =/= addr_ID && addr_MEM
=/= addr_ID (or equivalently check that addr_ID differs from both addr_EX and
addr_MEM) before setting amo_stall := false.B so hazards originating from
addr_ID are not masked.
Comment on lines
+230
to
+234
| val writeData = dontTouch(MuxCase(io.writeData, List( | ||
| (io.csr_Ex && io.id_ex_regWr && ((registerRs1 === io.id_ex_rd) || (registerRs2 === io.id_ex_rd)) && (io.id_ex_rd =/= 0.U)) -> io.csr_Ex_data, | ||
| (io.csr_Mem && io.ex_mem_regWr && ((registerRs1 === io.ex_mem_rd) || (registerRs2 === io.ex_mem_rd)) && (io.ex_mem_rd =/= 0.U)) -> io.csr_Mem_data, | ||
| (io.csr_Wb && io.ctl_writeEnable(0) && ((registerRs1 === io.writeReg) || (registerRs2 === io.writeReg)) && (io.writeReg =/= 0.U)) -> io.csr_Wb_data | ||
| ))) |
There was a problem hiding this comment.
Keep the CSR bypass selection per source operand.
This shared writeData helper switches to CSR data when either registerRs1 or registerRs2 matches a CSR producer, then the same helper is reused for rs1, rs2, and rs3 bypasses. A CSR match on one source can therefore feed the wrong value into the other sources. Please compute the forwarded value per operand instead of once globally.
Also applies to: 260-260, 279-279, 288-288
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/scala/components/InstructionDecode.scala` around lines 230 - 234,
The current shared writeData MuxCase wrongly selects CSR-forwarded data once
using a condition that ORs registerRs1 and registerRs2, then reuses writeData
for rs1/rs2/rs3 bypasses; change this so each source operand has its own
forwarded-value computation (e.g., writeDataRs1, writeDataRs2, writeDataRs3)
that checks only that operand against id_ex_rd/ex_mem_rd/writeReg and uses the
corresponding CSR data signals (io.csr_Ex_data, io.csr_Mem_data, io.csr_Wb_data)
and control flags (io.csr_Ex, io.csr_Mem, io.csr_Wb, io.id_ex_regWr,
io.ex_mem_regWr, io.ctl_writeEnable(0)) so a CSR hit on rs1 cannot pollute
rs2/rs3.
Comment on lines
+293
to
+301
| if (A) { | ||
| // AMO Forwarding (Memory Dependency) overrides Register Forwarding | ||
| when(hdu.io.operandForwardEX.get) { | ||
| io.readData1 := io.ex_result | ||
| io.readData2 := io.ex_result | ||
| }.elsewhen(hdu.io.operandForwardMEM.get) { | ||
| io.readData1 := io.ex_mem_result | ||
| io.readData2 := io.ex_mem_result | ||
| } |
There was a problem hiding this comment.
Only forward the AMO hazard value into rs1.
Here the AMO hazard path overwrites both operands, but hdu.io.addr_id is sourced from readData1 at Lines 435-437, and src/main/scala/components/HazardUnit.scala:76-151 detects AMO hazards by address match. So this forwarding condition is about the address register, not rs2. Clobbering io.readData2 will break AMOs/SCs whose data operand is unrelated.
Suggested fix
if (A) {
// AMO Forwarding (Memory Dependency) overrides Register Forwarding
when(hdu.io.operandForwardEX.get) {
io.readData1 := io.ex_result
- io.readData2 := io.ex_result
}.elsewhen(hdu.io.operandForwardMEM.get) {
io.readData1 := io.ex_mem_result
- io.readData2 := io.ex_mem_result
}
}Also applies to: 435-437
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/scala/components/InstructionDecode.scala` around lines 293 - 301,
The AMO forwarding path currently overwrites both operands (io.readData1 and
io.readData2); change the forwarding in the AMO hazard branches so only
io.readData1 is assigned from the forwarded value (use io.ex_result for
hdu.io.operandForwardEX.get and io.ex_mem_result for
hdu.io.operandForwardMEM.get), leaving io.readData2 untouched; apply the same
fix to the other AMO forwarding occurrence that mirrors these conditions (the
block that also checks hdu.io.operandForwardEX/operandForwardMEM and currently
writes both readData1/readData2), and ensure hdu.io.addr_id remains sourced from
the correct (possibly forwarded) readData1 only.
Comment on lines
47
to
55
| when(io.req.valid && !io.req.bits.isWrite) { | ||
| // READ | ||
| // rdata := mem.read(io.req.bits.addrRequest/4.U) | ||
| //validReg := true.B | ||
| sram.io.csb_i := false.B | ||
| sram.io.we_i := true.B | ||
| sram.io.addr_i := io.req.bits.addrRequest | ||
|
|
||
| io.rsp.valid := true.B | ||
| rdata := sram.io.rdata_o |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the sram_top.v for read latency (registered outputs)
echo "Searching for SRAM verilog implementation:"
fd -t f "sram_top.v" --exec cat {} \; 2>/dev/null | head -80Repository: merledu/nucleusrv
Length of output: 104
🏁 Script executed:
# Search for sram_top references in the codebase
rg "sram_top" --type scala --type v --type verilog -i
# Also search for the SRamTop.scala file to confirm its location
fd -t f "SRamTop.scala"Repository: merledu/nucleusrv
Length of output: 145
🏁 Script executed:
# Read the SRamTop.scala file
cat -n src/main/scala/components/SRamTop.scalaRepository: merledu/nucleusrv
Length of output: 3598
🏁 Script executed:
# Search for sram_top.v and sram.v files
fd -t f "sram_top.v" -o "sram.v"
# Also check in resources directory
find . -type d -name "resources" 2>/dev/nullRepository: merledu/nucleusrv
Length of output: 232
🏁 Script executed:
# List files in the resources directory
ls -la src/main/resources/
# Search for sram files
find src/main/resources -name "*.v" -type fRepository: merledu/nucleusrv
Length of output: 344
🏁 Script executed:
# Read the SRAM top wrapper
cat -n src/main/resources/sram_top.v
# Read the SRAM implementation
cat -n src/main/resources/sram.vRepository: merledu/nucleusrv
Length of output: 7361
Response valid asserted combinationally while SRAM read data has 1-cycle latency.
Setting io.rsp.valid := true.B combinationally on the same cycle as the read request is incorrect. The underlying sram module (in sram.v) uses a registered output for dout0 (line 59), which is updated via non-blocking assignment on the negedge clock (lines 132-136). This means the read data is only available in the next clock cycle, not the same cycle when rsp.valid is asserted. The response valid signal must be registered or delayed to match the data latency.
Additionally, line 55 redundantly reassigns rdata := sram.io.rdata_o, duplicating line 41.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/scala/components/SRamTop.scala` around lines 47 - 55, The response
valid is being asserted combinationally while sram.io.rdata_o is registered and
available next cycle; add a 1-cycle register (e.g., rspValidReg) that captures
the read-request condition (when io.req.valid && !io.req.bits.isWrite) and drive
io.rsp.valid := rspValidReg so valid aligns with data latency, and remove the
duplicate assignment to rdata by assigning sram.io.rdata_o to your single rdata
register only once (use the registered rdata output to update rdata on the next
clock edge); update the read-path handling in the same when block (referencing
io.req.valid, io.req.bits.isWrite, sram.io.rdata_o, rdata and io.rsp.valid) to
use the registered valid/data handshake.
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
2 participants
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.
Fixed the 2-cycle delay caused by the SRAM by:
SRAMTop.scalawrapper.FCSRforwarding issue.rs2forwarding issue for the A-Extension.Summary by CodeRabbit
Release Notes
Documentation
Configuration
Infrastructure