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

feat/impl riscv ADD instruction #85

Closed
wants to merge 16 commits into from

Conversation

KimiWu123
Copy link
Contributor

@KimiWu123 KimiWu123 commented Jul 5, 2024

Description

Implemented RISC-V ADD instruction. The implemenation is basically the same as ADD operation in EVM. The only difference is replacing stack operations with register operations. Another change is to isolate RISC-V instruction enumerations.

Changes

  • added Register gadget to manage register operations.
  • added risc-v related files to avoid touching existing code.
    • two enums,RV64IOpcode and RvInstructions in riscv_constant.rs
    • instructions_riscv_ext.rs to handle RISC-V related logic

Benchmark

Minor performance improved

EVM ADD  time:    [1.8772 s 1.8873 s 1.8961 s]
RISC-V ADD time:  [1.6199 s 1.6339 s 1.6495 s]
          change: [-14.351% -13.428% -12.589%]

@KimiWu123 KimiWu123 changed the title feat/impl risicv add [WIP] feat/impl risicv add Jul 8, 2024
@lispc lispc requested review from iammadab and dreamATD July 8, 2024 08:43
@lispc lispc changed the title [WIP] feat/impl risicv add [WIP] feat/impl riscv add Jul 8, 2024
Copy link
Collaborator

@hero78119 hero78119 left a 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/riscv_add.rs Outdated Show resolved Hide resolved
@@ -41,6 +41,9 @@ pub mod mstore;
// system
pub mod calldataload;

// riscv
pub mod riscv_add;
Copy link
Collaborator

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 under singer/src/instructions/riscv/
  • this mod can be toggle on/off via a feature flat riscv

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in d8b52ba

singer-utils/src/constants.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@hero78119 hero78119 left a 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

singer/src/instructions/riscv_add.rs Outdated Show resolved Hide resolved
singer/src/instructions/riscv_add.rs Outdated Show resolved Hide resolved
singer/src/instructions/riscv_add.rs Outdated Show resolved Hide resolved
@KimiWu123 KimiWu123 changed the title [WIP] feat/impl riscv add feat/impl riscv ADD instruction Jul 14, 2024
@KimiWu123 KimiWu123 requested a review from hero78119 July 14, 2024 04:26
@KimiWu123 KimiWu123 marked this pull request as ready for review July 14, 2024 04:26
@KimiWu123
Copy link
Contributor Author

Lint errors has been addressed. To make review easier, I'll commit it after this PR is approved

@hero78119 hero78119 requested a review from zemse July 23, 2024 08:20
Copy link
Collaborator

@hero78119 hero78119 left a 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

singer-utils/src/chip_handler.rs Show resolved Hide resolved
singer-utils/src/chip_handler/register.rs Outdated Show resolved Hide resolved
singer-utils/src/chip_handler/register.rs Outdated Show resolved Hide resolved
singer/src/instructions/riscv/add.rs Show resolved Hide resolved
Copy link
Collaborator

@hero78119 hero78119 left a 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!

singer-utils/src/chip_handler.rs Show resolved Hide resolved
@KimiWu123
Copy link
Contributor Author

Lint errors has been addressed. To make review easier, I'll commit it after this PR is approved

Format error has been fixed in cfe21fc, but still are some clippy errors. I think we can fix it in later PR.

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(
Copy link
Collaborator

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)

@KimiWu123
Copy link
Contributor Author

close it due to having new design #129

@KimiWu123 KimiWu123 closed this Aug 21, 2024
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