Corrected RISC-V shift word instructions #412
Merged
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: