Fix: Mask shift amount to 5 bits for SLL, SRL, and SRA instructions#1
Open
Fix: Mask shift amount to 5 bits for SLL, SRL, and SRA instructions#1
Conversation
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
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.
Bug Fix
First of all, congratulations on your work on this processor. You have used it to publish multiple articles in a relevant research area. My research focuses on verifying RISC-V processors, and I am using your design in my evaluation.
ProcessorCI
Our Article
The Issue
Currently, the shift logic in
alu.vhduses the entire 32-bit word of the rs2 operand (data2_i) to determine the shift amount. However, according to the RISC-V Unprivileged Architecture Specification (RV32I Base Integer Instruction Set), these instructions should only consider the lower 5 bits of the shift amount register.From the RISC-V Specification (v20250508):
"SLL, SRL, and SRA perform logical left, logical right, and arithmetic right shifts on the value in register rs1 by the shift amount held in the lower 5 bits of register rs2."
How to Reproduce
Using the full word causes incorrect results if software leaves non-zero values in the upper bits of the shift amount register. The bug was exposed after applying the
sra-01.elftest program from the riscv-arch-test repository. Specifically, this code snippet:The Fix
The shift amount is now explicitly masked to the 5 least significant bits (4 downto 0) before being converted to an integer. This ensures the hardware ignores the upper 27 bits of rs2 as required by the ISA.