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

Implement atomic instructions(A Standard Extension) #329

Merged
merged 12 commits into from
Mar 25, 2023

Conversation

mohanson
Copy link
Collaborator

@mohanson mohanson commented Feb 28, 2023

  • Update CI with A Extension testsuite
  • ckb_vm::run and examples/ckb-vm-runner adds ISA_A by default

@mohanson mohanson requested a review from xxuejie February 28, 2023 03:36
src/machine/mod.rs Outdated Show resolved Hide resolved
src/machine/asm/execute_x64.S Outdated Show resolved Hide resolved
CHECK_WRITE(RS1, TEMP1d, 4)
movslq CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_MEMORY(MACHINE, RS1), TEMP1
WRITE_RD(TEMP1)
add TEMP1, RS2r
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed a line in RISC-V's spec:

For RV64, 32-bit AMOs always sign-extend the value placed in rd, and ignore the upper 32 bits of the original value of rs2

Does this mean we need to clear the upper 32 bits of rs2 here for all AMO*.W operations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After add TEMP1, RS2r, we write the result to memory using the following statement:

mov RS2rd, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_MEMORY(MACHINE, RS1)

It has no affects the final result whether or not the upper 32 bits of rs2 are cleared.

Copy link
Collaborator

Choose a reason for hiding this comment

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

2 things:

  • The spec requires rd to be sign-extended, which means mov RS2rd might not be correct here?
  • For add you are correct, this might end up in the same result, but what about MIN/MAX? Ignoring upper 32 bits or sign-extending can lead to different result, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • mov RS2rd, CKB_VM_ASM_ASM_CORE_MACHINE_OFFSET_MEMORY(MACHINE, RS1) moves RS2rd to memory, not rd, and rd = sign_ext(memory[rs1])
  • Good question, I need to take a closer look.

@xxuejie
Copy link
Collaborator

xxuejie commented Mar 16, 2023

One more thing: it seems the new issues we found here are not covered by riscv-tests, any chance we can add some unit tests covering checks on those behavior?

@mohanson
Copy link
Collaborator Author

mohanson commented Mar 16, 2023

One more thing: it seems the new issues we found here are not covered by riscv-tests, any chance we can add some unit tests covering checks on those behavior?

Yes, I'm going to create some PRs to riscv-tests, but before that I will add tests for ckb-vm.

@mohanson mohanson merged commit d3d7fc7 into nervosnetwork:develop Mar 25, 2023
@mohanson mohanson deleted the isa_a branch March 25, 2023 11:28
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.

2 participants