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

chore: refactor build_decoder #313

Closed

Conversation

quake
Copy link
Member

@quake quake commented Dec 9, 2022

move the isa and version checking to fn build_decoder, avoid duplicate code on different machine building fn, rvv extensions and version checking can also be handled centrally in build_decoder

@xxuejie
Copy link
Collaborator

xxuejie commented Dec 12, 2022

I have a tangent question relating to decoder code: why is isa a plain u8 using masks to represent different configurations? I can get that version is an ever-increasing value, hence u32 fits the case. But ckb-vm is first a RISC-V interpreter, it is optimized for CKB but could be used in more general use case. Here a u8 could be limiting things quite a bit: what if I just want IM but not C? When V is merged, what if I want to set VLEN to 32, not 1024?

I don't recall we use isa in any of the assembly code(do we?), so what if we refactor isa into a proper Rust enum? That could bring much flexibility and also type safety to the code. Thinking further: should register width belong to a generic type parameter as they do now, or should they also belong to isa here?

@mohanson
Copy link
Collaborator

Defining the ISA as an enum makes sense in my opinion. The ISA only acts on the decoder, so its performance is not important.

I don't know how necessary it is to set vlen and v register width dynamically. One of the goals of rvv is that the same rvv code can run on different rvv widths. Providing a larger vlen and width has only advantages and no disadvantages.

@xxuejie
Copy link
Collaborator

xxuejie commented Dec 12, 2022

I do admit that providing larger VLEN can indeed enable more programs, but it really depends on your goal: if you are aiming at building a generic RISC-V implementation, it will be important to support different VLEN, in reality there might be different VLEN configurations developers want to test against, not to mention the fact that a significant portion of instructions implemented now on ckb-vm is still defined as reserved, no actual processors use them. In fact, having more VLEN than what's feasible now, can sometimes also be a burden: a program runs on CKB-VM, but they might not run on the outside world. For a soft-core, it will be beneficial if it can resemble real hardware as close as possible

@mohanson
Copy link
Collaborator

Just considering the PR itself, the code is fine, so I'll be ready to merge. Any other ideas? @xxuejie

@xxuejie
Copy link
Collaborator

xxuejie commented Dec 15, 2022

Personally if you ask me, I prefer a more complete PR to refactor isa part mentioned above, but I won't object if you really want to merge this.

@mohanson mohanson closed this Feb 9, 2023
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