Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Corrected RISC-V shift word instructions #412

Merged
merged 1 commit into from
May 21, 2024
Merged

Conversation

JosephMoore25
Copy link
Contributor

I spotted that the RISC-V instructions for shifting words have a minor inaccuracy. In the 64-bit variants, we take the lowest 6 bits (up to 63) for the shamt (shift-amount). In the word (32 bit) variants, we should only be taking the lowest 5 bits (up to 31) for shamt, as there are only 31 bit positions to shift before you've looped back.

Functionally, this inaccuracy caused no issues due to the cyclic nature of shifts. If you are to shift a 32 bit number by shamt=34, this is equivalent to shamt=2. Due to this, all tests previously passed and still do pass. For the sake of being spec accurate, this change should be made to limit the shifts of words to 31. No test can be written to cover this due to them being functionally identical.

I've verified for each instruction that this is correct compared to the RISC-V unprivileged spec, SAIL, and the spike implementation of these instructions.

From the spec:

In RV64I, only the low 6 bits of rs2 are considered for the shift amount.
SLLW, SRLW, and SRAW are RV64I-only instructions that are analogously defined but operate
on 32-bit values and produce signed 32-bit results. The shift amount is given by rs2[4:0].

@JosephMoore25 JosephMoore25 self-assigned this May 1, 2024
@JosephMoore25 JosephMoore25 added the code consistency Cleaning up existing code to align with rest of project label May 1, 2024
Copy link
Contributor

@ABenC377 ABenC377 left a comment

Choose a reason for hiding this comment

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

👌🏻

Copy link
Contributor

@dANW34V3R dANW34V3R left a comment

Choose a reason for hiding this comment

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

Looks good. Some "discrete" wording in the spec for these

@JosephMoore25 JosephMoore25 merged commit eed073e into dev May 21, 2024
2 checks passed
ABenC377 pushed a commit that referenced this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code consistency Cleaning up existing code to align with rest of project
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants