Skip to content

Conversation

eleanorLYJ
Copy link
Collaborator

@eleanorLYJ eleanorLYJ commented Feb 8, 2025

According to RV32E Base Integer Instruction Set, Version 1.9 [1]:

RV32E can be combined with all current standard extensions. Defining the F, D, and Q extensions as having a 16-entry floating point register file when combined with RV32E was considered but decided against.

When RV32E is used with floating-point extensions, the floating-point register file should have 32 entries instead of N_RV_REGS (16).

Adjust array size accordingly.

[1] https://five-embeddev.com/riscv-user-isa-manual/Priv-v1.12/rv32e.html#rv32e

Summary by Bito

Fixed a critical bug in RISC-V implementation where floating-point register array size was incorrectly set to 16 for RV32E with F extension. Updated riscv_private.h to maintain 32 floating-point register entries as per RISC-V specification when RV32E is combined with floating-point extensions.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 1

According to RV32E Base Integer Instruction Set, Version 1.9 [1]:

RV32E can be combined with all current standard extensions. Defining the
F, D, and Q extensions as having a 16-entry floating point register file
when combined with RV32E was considered but decided against.

When RV32E is used with floating-point extensions, the floating-point
register file should have 32 entries instead of N_RV_REGS (16).

Adjust array size accordingly.

[1] https://five-embeddev.com/riscv-user-isa-manual/Priv-v1.12/rv32e.html#rv32e
Copy link

bito-code-review bot commented Feb 8, 2025

Code Review Agent Run #20c403

Actionable Suggestions - 0
Additional Suggestions - 1
  • src/riscv_private.h - 1
    • Consider using N_RV_REGS constant for consistency · Line 134-134
Review Details
  • Files reviewed - 1 · Commit Range: afbb5b4..afbb5b4
    • src/riscv_private.h
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by Bito Logo

Copy link

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Bug Fix - RISC-V Floating-Point Register Count Correction

riscv_private.h - Fixed floating-point register array size from N_RV_REGS to 32 for RV32E with F extension

@jserv jserv requested a review from visitorckw February 9, 2025 08:23
Copy link
Collaborator

@visitorckw visitorckw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thx!

@jserv jserv merged commit 3fbcfbf into sysprog21:master Feb 9, 2025
5 of 8 checks passed
@jserv
Copy link
Contributor

jserv commented Feb 9, 2025

Thank @eleanorLYJ for contributing!

@jserv jserv added this to the release-2025.1 milestone Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants