-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat/impl riscv ADD instruction #85
Conversation
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.
Just left few comments :)
also discussed with @KimiWu123 that we can add benchmarks for riscv add
to have a insightful on evm_add 256 bits stack operation vs riscv64 register operation comparison
singer/src/instructions.rs
Outdated
@@ -41,6 +41,9 @@ pub mod mstore; | |||
// system | |||
pub mod calldataload; | |||
|
|||
// riscv | |||
pub mod riscv_add; |
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.
Few advices to isolated different ISA implementation
- define a new package
riscv
for all riscv instruction, whcih can be undersinger/src/instructions/riscv/
- this mod can be toggle on/off via a feature flat
riscv
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.
fixed in d8b52ba
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.
few suggested comments for naming
919510b
to
56ada4c
Compare
4f70bae
to
26554f2
Compare
d8b52ba
to
4a35061
Compare
9c4be95
to
83cd8b0
Compare
Lint errors has been addressed. To make review easier, I'll commit it after this PR is approved |
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.
Hi, just recalled and left few comment related to offline memory checking protocol correctness
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.
Except for a nitpick, other LGTM!
Format error has been fixed in cfe21fc, but still are some |
let prev_rs2_ts = (&phase0[Self::phase0_prev_rs2_ts()]).try_into()?; | ||
let prev_rd_ts = (&phase0[Self::phase0_prev_rd_ts()]).try_into()?; | ||
let memory_ts = (&phase0[Self::phase0_memory_ts()]).try_into()?; | ||
TSUInt::assert_lt( |
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.
Thanks for @kunxian-xia to point out that rs1
, rs2
, rd
might be the same registers follow riscv specs, so we should bump next_memory_ts
accordingly within opcode. See the discussion #91 (comment)
close it due to having new design #129 |
Description
Implemented RISC-V
ADD
instruction. The implemenation is basically the same asADD
operation in EVM. The only difference is replacing stack operations with register operations. Another change is to isolate RISC-V instruction enumerations.Changes
Register
gadget to manage register operations.RV64IOpcode
andRvInstructions
inriscv_constant.rs
instructions_riscv_ext.rs
to handle RISC-V related logicBenchmark
Minor performance improved