-
Notifications
You must be signed in to change notification settings - Fork 172
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
Add Zvbb extension #558
base: master
Are you sure you want to change the base?
Add Zvbb extension #558
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.
Looks good overall, I left some small comments.
I'll have to give that vector test generator a go, I've been trying better understand the vector extension and our implementation recently.
model/riscv_insts_zvbb.sail
Outdated
let vs2_val_vec : vector('n, dec, bits('m)) = read_vreg(num_elem, SEW, LMUL_pow, vs2); | ||
let vd_val : vector('n, dec, bits('o)) = read_vreg(num_elem, SEW_widen, LMUL_pow, vd); | ||
|
||
(result, mask) = init_masked_result(num_elem, SEW_widen, LMUL_pow - 1, vd_val, vm_val); |
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.
I recently refactored this into:
let (initial_result, mask) = init_masked_result(num_elem, SEW_widen, LMUL_pow - 1, vd_val, vm_val);
var result = initial_result;
elsewhere, so we can remove the = undefined
lines. Would be good to make this consistent.
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.
(also same for the above instructions)
Makefile
Outdated
@@ -128,8 +130,6 @@ SAIL_RMEM_SRCS = $(addprefix model/,$(SAIL_ARCH_SRCS) $(SAIL_RMEM_INST_SRCS) $(S | |||
SAIL_RVFI_SRCS = $(addprefix model/,$(SAIL_ARCH_RVFI_SRCS) $(SAIL_SEQ_INST_SRCS) $(RVFI_STEP_SRCS)) | |||
SAIL_COQ_SRCS = $(addprefix model/,$(SAIL_ARCH_SRCS) $(SAIL_SEQ_INST_SRCS) $(SAIL_OTHER_COQ_SRCS)) | |||
|
|||
PLATFORM_OCAML_SRCS = $(addprefix ocaml_emulator/,platform.ml platform_impl.ml softfloat.ml riscv_ocaml_sim.ml) |
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.
Should these OCaml removal changes be part of this PR? We recently removed it the OCaml simulator, not sure if these were missed or just an artifact of a rebase commit.
model/riscv_insts_zvbb.sail
Outdated
enum clause extension = Ext_Zvbb | ||
function clause extensionEnabled(Ext_Zvbb) = true | ||
|
||
mapping vm_name : bits(1) <-> string = { |
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.
Wondering if we have this mapping already somewhere?
@Alasdair I mistakenly used the riscv-vector-tests to generate ELF files with vlen=256, which differ from the vlen=512 in sail-riscv. After rerunning the tests, I discovered that there is a difference in the new_vl calculation between the implementations. The riscv-vector-tests use VLMAX, while sail-riscv use ceil(AVL / 2).
resulted in successful test execution when change the |
I noticed the vctz vclz is missing. So I added. |
And vcpop |
I noticed that PR #236 does not yet implement the vwsll instruction. However, based on the latest draft, this instruction has recently been updated. The ZvkB extension has now been split into Zvbc and Zvbb.
The following variants of the vwsll instruction have been included:
vwsll [vv,vx,vi]
Additionally, I have successfully passed all vwsll tests from the RISC-V vector tests repository.