-
Notifications
You must be signed in to change notification settings - Fork 21
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
Implement the TODOs in the Harvard Architecture PR #711
Implement the TODOs in the Harvard Architecture PR #711
Conversation
22b7a0c
to
565665d
Compare
565665d
to
3031a63
Compare
3031a63
to
f441913
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The LUI circuit is more efficient than ADDI. No register read, fewer range checks, no addition... I recommend putting it back. You can implement AUIPC with the LUI circuit though (absolute addressing). |
If you think it's worth the hassle, we can restore the LUI circuit and also compile ADDI with rs1=0 to LUI. |
Indeed. |
In the Harvard Architecture PR we left a few TODOs. Here we make good on them.
Alas, unlike @icemelon's hopes the change is slightly more involved.
There's a few things happening in this PR:
We remove the fake instruction
EANY
.EANY
was only ever an artifact of Risc0's incredible 'interesting' approach to decoding. We use the real instructionsECALL
andEBREAK
instead.Remove
AUIPC
andLUI
. Both of them are now implemented as pseudo-instructions, that translate toADDI
during decoding.Use the same library as SP1 for RiscV decoding, instead of copy-and-pasting-and-editing Risc0's 'interesting' decoder. That simplifies our code, and comes with a lot more tests than we ever had. Both because of explicit tests in the library, and because of the usage in SP1 and other projects. This gets rid of much error prone bit manipulating code.
Use
struct Instruction
throughout the code when handling and testing instructions, instead ofu32
. That makes specifying tests a lot simpler and more readable. No more0b_000000001010_00000_000_00001_0010011, // addi x1, x0, 10
in the code.Remove the notion of executable vs non-executable ROM. This is only necessary for a von-Neumann architecture: everything that's in our instruction-cache is meant to be executable already. (We can re-implement this restriction later by controlling what is allowed to make it into the instruction cache when we eg decode the ELF. But it's unnecessary: we already honour the executable flag for memory sections in the ELF.)
Please review with whitespace changes hidden for an easier reading experience.