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

Add Zvbb extension #558

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Add Zvbb extension #558

wants to merge 14 commits into from

Conversation

rez5427
Copy link
Contributor

@rez5427 rez5427 commented Sep 22, 2024

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.

Copy link

github-actions bot commented Sep 22, 2024

Test Results

396 tests  ±0   396 ✅ ±0   0s ⏱️ ±0s
  4 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 037daa6. ± Comparison against base commit fd1be4b.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@Alasdair Alasdair left a 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.

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);
Copy link
Collaborator

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.

Copy link
Collaborator

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

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.

enum clause extension = Ext_Zvbb
function clause extensionEnabled(Ext_Zvbb) = true

mapping vm_name : bits(1) <-> string = {
Copy link
Collaborator

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?

@rez5427
Copy link
Contributor Author

rez5427 commented Sep 25, 2024

@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).

val calculate_new_vl : (int, int) -> xlenbits
function calculate_new_vl(AVL, VLMAX) = {
/* Note: ceil(AVL / 2) ≤ vl ≤ VLMAX when VLMAX < AVL < (2 * VLMAX)
* TODO: configuration support for either using ceil(AVL / 2) or VLMAX
*/
if AVL <= VLMAX then to_bits(sizeof(xlen), AVL)
else if AVL < 2 * VLMAX then to_bits(sizeof(xlen), (AVL + 1) / 2)
else to_bits(sizeof(xlen), VLMAX)
}

resulted in successful test execution when change the to_bits(sizeof(xlen), (AVL + 1) / 2) to to_bits(sizeof(xlen), VLMAX)

@rez5427 rez5427 changed the title Add Zvbb extension's vwsll instruction Add Zvbb extension Oct 8, 2024
@rez5427
Copy link
Contributor Author

rez5427 commented Oct 8, 2024

I noticed the vctz vclz is missing. So I added.

@rez5427
Copy link
Contributor Author

rez5427 commented Oct 10, 2024

And vcpop

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