Skip to content

Conversation

@WayfaringKid
Copy link
Contributor

@WayfaringKid WayfaringKid commented Nov 4, 2024

The encoding for the addiw instruction did not match the specification.

Previously, we had:
[imm:12][rs1:5]000[rd:5]0001011
Corrected to:
[imm:12][rs1:5]000[rd:5]0011011

Please refer to page 575 of the specification for more details.

Thank you!

@ThinkOpenly
Copy link
Collaborator

Proposed change matches what's in Chapter 34 of the 20240411 version of The RISC-V Instruction Set Manual Volume I: Unprivileged Architecture:
image

@dhower-qc
Copy link
Collaborator

Thanks for the contribution.

@AFOliveira, this reminds me that we should get the checker that validates against riscv-opcodes in the repo and running as a regression test.

@AFOliveira
Copy link
Collaborator

@AFOliveira, this reminds me that we should get the checker that validates against riscv-opcodes in the repo and running as a regression test.

We surely do. How do you prefer this done? I can create a script that simply prints the matches from riscv-opcodes by instruction. The thing is will all parts need to be done in ruby? Or can the match be generated from a python script and the comparison in ruby?

@dhower-qc
Copy link
Collaborator

Python is fine. The only Ruby part that will be needed is a task to run the python script with the rest of the regression tests.

Look for task :regress in Rakefile; that's where you'll add a new regression task. The task itself should be pretty straightforward, e.g.:

  # this will fail if the python script exists with non-zero
  task :rv_op_check do
    sh "python BLAH BLAH"
  end

@dhower-qc dhower-qc merged commit c79a3a5 into riscv-software-src:main Nov 4, 2024
1 check passed
@ThinkOpenly
Copy link
Collaborator

I'd argue that verifying against riscv-opcodes is short-lived, as we really want to generate the content in risv-opcodes from the content in riscv-unified-db, but it could be a decent intermediate effort. (I think it'll eventually be thrown away, though.)

We've got a similar, but more challenging effort underway: to verify the content of riscv-unified-db from the content in the Sail model. That's how this issue was discovered. Unfortunately, our task is currently showing more errors in our JSON than the YAML, but we're working on it. ;-)

I've got a mentee looking at generating the content in riscv-opcodes from the YAML here, just started. 🤞

@AFOliveira
Copy link
Collaborator

AFOliveira commented Nov 4, 2024

I'd argue that verifying against riscv-opcodes is short-lived, as we really want to generate the content in risv-opcodes from the content in riscv-unified-db, but it could be a decent intermediate effort. (I think it'll eventually be thrown away, though.)

I think the take in that what goes in favor of that are two thing

  • It's pretty much already done
  • It will always be useful to verify for instructions that won't go anywhere, i.e. the base Isa. It surely has the bad part that if we can replace it, we won't have new instructions updated there, so it has limited use-cases

IMO checking from SAIL will be better of course, but since this won't take much time, it still has it's short-time purpose. I guess while it exists, we can double check. In the long-term I would suggest aditionally checking from binutils. This kind of checks will be great not only fot the udb, but also for SAIL, binutils and riscv-opcodes. By actually checking them all against each other, we'll ultimately enhace the whole ecossystem.

I've got a mentee looking at generating the content in riscv-opcodes from the YAML here, just started.

There is a branch in this repo called pr/AFOliveira/21 where I do the exact opposite. It is not fully up to date, but I'll try to make it soon. Alhtough, it's not the best written and careful code, it might be worth the check. If there is something I can help with that, feel free to hit me up!

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.

4 participants