Corrected RISC-V shift word instructions#412
Merged
JosephMoore25 merged 1 commit intodevfrom May 21, 2024
Merged
Conversation
FinnWilkinson
approved these changes
May 1, 2024
dANW34V3R
approved these changes
May 9, 2024
Contributor
dANW34V3R
left a comment
There was a problem hiding this comment.
Looks good. Some "discrete" wording in the spec for these
jj16791
approved these changes
May 10, 2024
JosephMoore25
added a commit
that referenced
this pull request
May 21, 2024
ABenC377
pushed a commit
that referenced
this pull request
May 24, 2024
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.
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) forshamt, 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 toshamt=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: