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

Fix calculation of shift amount for vector single-width shift instructions #9

Merged
merged 1 commit into from
Aug 29, 2024

Conversation

danielschloms
Copy link

Description

According to the current RISC-V ISA manual, only the lower $log_2(SEW)$ bits of the shift amount are used to determine the actual shift amount for single-width vsll, vsrl, and vsra. Currently, due to an extra loop iteration, one extra bit is included. This PR addresses this issue by simply shifting target_width_bits right by one before the loop.

@PhilippvK
Copy link
Member

@danielschloms Sorry, I never got notified for this…

@JoGei Would you also like to take a look? (Should be a trivial change, though)

@PhilippvK
Copy link
Member

@danielschloms Seems like CI failed (I am note quite sure if it was fine the last time I worked on the repo, hence it could also be an existing bug). Are you able to run the test locally to verify what is going on?

@danielschloms
Copy link
Author

danielschloms commented Aug 28, 2024

@PhilippvK Locally most of the unit tests fail, but they also do so when compiling from rvv1.0. With --output-on-failure both branches produce the exact same result for me.

@JoGei
Copy link
Member

JoGei commented Aug 29, 2024

Hi, yes you are correct @danielschloms
This was also already stated in the original rvv0.9 spec so wrong in sofvector.

How did you encounter this, though? Am I correct with this example:

  • SRL, SEW=8, shift amount passed as 9=0b1001
  • [bug impl, current] vd[x] = vs[x] >> 9 which means vd[x] = 0 for all elements x of vs because we would "overshift"
  • [correct] vd[x] = vs[x] >> (0b1001 & 0b111) = vd[x] = vs[x] >> 1 for all elements x of vs

@danielschloms
Copy link
Author

@JoGei Seems right to me. I found this when debugging these RISC-V tests with ETISS, where the expected values matched the specification but the tests failed.

@JoGei
Copy link
Member

JoGei commented Aug 29, 2024

@JoGei Seems right to me. I found this when debugging these RISC-V tests with ETISS, where the expected values matched the specification but the tests failed.

Okay, but the test suite seems to be wrong, too? @PhilippvK ?
https://github.com/PhilippvK/riscv-tests/blob/47d33635daf784ab65c488dbfd7103efe8e4e09c/isa/rv32uv/vsll.c#L12-L17

Note the expected result for the element 7 and 15 (0x1 << 32, e8 ). The test expects: 0x80, which is 0x1<<7, however, 32 should result in a shift amount of 0 since SEW=e8 dictates only the first 3 bits (all 0) to be used?

@danielschloms
Copy link
Author

@JoGei I think you have it misaligned:
In: 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01, 0x01
<<:0, 1, 2, 3, 7, 15, 31, 32, 0, 1, 2, 3, 7, 15, 31, 32
Re:0x01, 0x02, 0x04, 0x08, 0x80, 0x80, 0x80, 0x01, 0x01, 0x02, 0x04, 0x08, 0x80, 0x80, 0x80, 0x01

@JoGei JoGei merged commit 2972d9e into tum-ei-eda:rvv1.0 Aug 29, 2024
1 check failed
@danielschloms danielschloms deleted the fix-get-shiftamount branch September 9, 2024 14:36
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