Support partial vector extension instructions #545

vestata · 2025-01-24T14:54:11Z

Add support for the RISC-V "V" Vector Extension. This pull request implements decoding for 585 out of 616 version 1.0 spec vector instructions, with partial interpreter implementation.

The decoding method for vector instructions, including vector configuration and load/store instructions, follows the approach used in rv32emu. The new rvv_jumptable is introduced to handle remaining arithmetic instructions.

The interpreter implementation is tested using the riscv-vector-tests repository, with current limitations, as outlined in the repo. Included partial support for vector load/store instructions and single-width arithmetic instructions. The architecture now supports different settings for sew, lmul, and vector masking.

Vector instructions passing the tests include:

vle8.v, vle16.v, vle32.v
vse8.v, vse16.v, vse32.v
vadd.vv, vadd.vx, vadd.vi
vsub.vv, vsub.vx, vsub.vi,
vand.vv, vand.vx, vand.vi
vor.vv, vor.vx, vor.vi
vxor.vv, vxor.vx, vxor.vi
vsll.vv, vsll.vx, vsll.vi
vmul.vv, vmul.vx, vmul.vi

Close #504

Summary by Bito

This pull request implements extensive support for the RISC-V Vector Extension, decoding 585 out of 616 vector instructions. It enhances vector load/store operations, arithmetic instructions, and configuration management, while introducing a new jump table for remaining instructions. Many operations are still placeholders, indicating areas for future development.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 2

jserv

Benchmarks

Benchmark suite	Current: `c3374ea`	Previous: `4ef61b8`	Ratio
`Dhrystone`	`1256` Average DMIPS over 10 runs	`1284` Average DMIPS over 10 runs	`1.02`
`Coremark`	`946.035` Average iterations/sec over 10 runs	`972.508` Average iterations/sec over 10 runs	`1.03`

This comment was automatically generated by workflow using github-action-benchmark.

src/decode.c

jserv · 2025-01-24T15:59:33Z

src/decode.c


-    /* standard uncompressed instruction */
-    const uint32_t index = (insn & INSN_6_2) >> 2;
+static inline bool op_000000(rv_insn_t *ir, const uint32_t insn)


op_000000 looks misleading. Can you improve its naming scheme?

The naming scheme is based on the function6 field listed in riscv-v-spec/inst-table.adoc. Since each function6 may include OPI, OPM, or OPF functions, often corresponding to unrelated operations. I chose to name them directly based on the function6 for consistency.

This might seem unclear without additional context. To improve clarity, I could add comments explaining the naming convention for each op_function6. Would this address your concern?

src/riscv.h

vacantron · 2025-01-24T16:35:00Z

The interpreter implementation is tested using the riscv-vector-tests repository

Could we create an CI like using ROSCOF for this?

vestata · 2025-01-25T11:05:44Z

The interpreter implementation is tested using the riscv-vector-tests repository

Could we create an CI like using ROSCOF for this?

I'm not familiar with ROSCOF, but I'll look into it and give it a try.

src/riscv_private.h

src/emulate.c

eleanorLYJ · 2025-01-25T14:24:01Z

Suggest using git rebase -i to squash the commit into the previous one instead of adding a new commit.

src/rv32_template.c

.gitignore

src/decode.c

src/decode.h

src/decode.c

src/decode.h

vestata · 2025-01-26T14:39:37Z

Thank you all for your feedback and suggestions! I will fix the typos, add a newline at the end of files, and remove any unnecessary elements. I also noticed that the current code does not fully meet the contributing guidelines, so I will make sure to address those issues. In addition, I will add more detailed comments in src/decode.c and src/rv32_template.c and ensure the formatting is correct.

Since some of the code was misplaced from the beginning, and as @eleanorLYJ mentioned, there are non-compliant comments in an early commit, I’m considering git rebase -i everything from the start. Do you have any suggestions or concerns about that approach?

I’d appreciate your guidance. Thank you!

howjmay · 2025-01-27T00:19:52Z

src/rv32_template.c

+    }                                                                        \
+}
+
+#define VMV_LOOP(des, op1, op2, op, SHIFT, MASK, i, j, itr, vm)             \


may I ask where this is one used?

The VMV_LOOP macro is used in the implementation of vmv_v_i(at src/rv32_template.c, line 6366), as the riscv-vector-tests frequently utilize vmv_v_i to clear bits in vector registers during each test. This serves as a quick implementation for the vmv_v_i instruction.

Additionally, I noticed that the implementations of vmv_v_* (representing vmv_v_v, vmv_v_x, and vmv_v_i) can be refactored to reuse existing macros such as VV_LOOP, VX_LOOP, VI_LOOP, and their _LEFT variants (collectively referred to as V*_LOOP and V*_LOOP_LEFT). I will remove the VMV_LOOP and related _LEFT macros accordingly. Thank you for pointing this out!

src/decode.c

bito-code-review · 2025-02-12T19:06:14Z

src/decode.c

+    case 5: /* Reserved */
+        decode_vxtype(ir, insn);
+        ir->opcode = rv_insn_vfmin_vf;
+        break;


Inconsistent reserved case implementation

The code marks case 5 as /* Reserved */ but then proceeds to implement it. Consider either removing the comment or making it a reserved case that returns false.

Code suggestion

Check the AI-generated fix before applying

Suggested change

case 5: /* Reserved */

decode_vxtype(ir, insn);

ir->opcode = rv_insn_vfmin_vf;

break;

case 5: /* Reserved */

Code Review Run #6921bb

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

bito-code-review · 2025-02-13T22:45:59Z

Code Review Agent Run #0be539

Actionable Suggestions - 3

src/rv32_constopt.c - 1
- Consider relocating endif directive placement · Line 1241-1241
src/rv32_template.c - 1
- Consider proper error handling over assertions · Line 7060-7063
src/decode.c - 1
- Consider using named constant for bitmask · Line 2470-2470

Additional Suggestions - 7

src/decode.c - 3
- Consider consolidating duplicate case statements · Line 3945-3947
- Consider consolidating vector instruction decode functions · Line 4215-4322
- Consider more descriptive function name · Line 2476-2476
src/decode.h - 3
- Consider using bit fields for vector flags · Line 959-959
- Consider splitting vector instructions into module · Line 234-425
- Consider consolidating vector floating-point instructions · Line 801-828
src/rv32_template.c - 1
- Consider consolidating vlmax calculation shifts · Line 3073-3076

Review Details

Files reviewed - 8 · Commit Range: 1737e76..464c22c
- Makefile
- src/decode.c
- src/decode.h
- src/feature.h
- src/riscv.h
- src/riscv_private.h
- src/rv32_constopt.c
- src/rv32_template.c
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Fb Infer (Static Code Analysis) - ✖︎ Failed

AI Code Review powered by

src/rv32_constopt.c

src/rv32_template.c

bito-code-review · 2025-02-13T22:59:41Z

src/decode.c

-        return op(ir, insn);
+static inline bool op_vcfg(rv_insn_t *ir, const uint32_t insn)
+{
+    switch (insn & 0x80000000) {


Consider using named constant for bitmask

Consider using a more descriptive constant for the bit mask 0x80000000. A named constant like VSETVLI_MASK would improve code readability and maintainability.

Code suggestion

Check the AI-generated fix before applying

Suggested change

switch (insn & 0x80000000) {

/* Mask for bit 31 of vector configuration instructions */

#define VSETVLI_MASK 0x80000000

switch (insn & VSETVLI_MASK) {

Code Review Run #0be539

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

howjmay · 2025-02-16T23:10:45Z

src/rv32_template.c

+            rv->csr_vtype = 0x80000000;
+            return true;
+        }
+        uint16_t vlmax = (v_lmul < 4)


Maybe some comment for this part?

Got it. I'll add some detail to the vlmax calculation.

howjmay · 2025-02-16T23:14:38Z

src/rv32_template.c

+
+/* clang-format off */
+
+#define OPT(des, op1, op2, op, op_type) {                                    \


Not a problem, but on thing I concern is some more complex RVV instructions can't be simply implemented by this macro.
These kinds of instructions include but not limit to vmerge, nclip, etc.
Not sure what is your plan for these instructions.
And maybe rename or add some comment to identify the limitation of OPT().

The name OPT is not precise. It is a matter of sew variant.

Not a problem, but on thing I concern is some more complex RVV instructions can't be simply implemented by this macro.
These kinds of instructions include but not limit to vmerge, nclip, etc.
Not sure what is your plan for these instructions.
And maybe rename or add some comment to identify the limitation of OPT().

You're right. The current implementation doesn't support widening/narrowing vector instructions, as well as vmerge and vnclip. The original plan is to introduce variants, such as WV_LOOP, to handle these cases. However, this PR focuses on the basic implementation of vector instructions, so I'll leave this for future work.

And maybe rename or add some comment to identify the limitation of OPT().

The name OPT is not precise. It is a matter of sew variant.

I'll rename OPT to VECTOR_DISPATCH, but let me know if you have a better suggestion.

howjmay · 2025-02-16T23:15:33Z

In general is an amazing PR. You made some incredible contribution

src/rv32_template.c

Add decode stage for RISC-V "V" Vector extension instructions from version 1.0, excluding VXUNARY0, VRFUNARY0, VWFUNARY0, VFUNARY1, vmv<nr>r, and VFUNARY0. This commit focuses on the decode stage to ensure correct instructions parsing before proceeding to the execution stage. Verification is currently done through hand-written code. Modify Makefile to support VLEN configuration, via make ENABLE_EXT_V=1 VLEN=<value>. The default value for VLEN is set to 128. The current implementation only supports VLEN=128. Enabling ENABLE_EXT_V=1 will also enable ENABLE_EXT_F=1, as vector load/ store instruction shares the same opcode with load_fp and store_fp.

Add support for vset{i}vl{i} instructions following the RISC-V vector extension version 1.0. Simplify avlmax calculation by directly computing avlmax = lmul * vlen / sew instead of converting to floating-point as described in the specification.

Implement vle8_v, vle16_v, vle32_v, vse8_v, vse16_v, vse32_v. Using loop unrolling technique to handle a word at a time. The implementation assumes VLEN = 128. There are two types of illegal instructions: 1. When eew is narrower than csr_vl. Set vill in vtype to 1 and other bits to 0, set csr_vl to 0. 2. When LMUL > 1 and trying to access a vector register that is larger than 31. Use assert to handle this case.

To emulate vector registers of length VLEN using an array of uint32_t, we first handle different SEW values (8, 16, 32) using sew_*b_handler. Inside the handler, the V*_LOOP macro expands to process different VL values and operand types, along with its corresponding V*_LOOP_LEFT. The goal is to maximize code reuse by defining individual operations next to their respective vector instructions, which can be easily applied using the OPT() macro. V*_LOOP execution steps: 1. Copy the operand op1 (op2). 2. Align op1 to the right. 3. Perform the specified operation between op1 and op2. 4. Mask the result according to the corresponding SEW. 5. Shift the result left to align with the corresponding position. 6. Accumulate the result. In vector register groups, registers should follow the pattern v2*n, v2*n+1 when lmul = 2, etc. The current implementation allows using any vector registers except those exceeding v31. For vector masking, if the corresponding mask bit is 0, the value of the destination vector register is preserved. The process is as follows: 1. Copy the destination register. 2. Clear the bits corresponding to VL. 3. Store the computed result in ans. 4. Update the destination register with ans. If ir->vm == 0, vector masking is activated.

visitorckw · 2025-02-18T09:04:39Z

The interpreter implementation is tested using the riscv-vector-tests repository

Could we create an CI like using ROSCOF for this?

I'm not familiar with ROSCOF, but I'll look into it and give it a try.

The current riscv-arch-test does not include a test suite for vector extension. However, we could explore using riscv-ctg to generate a suitable test suite.

ChinYikMing · 2025-02-18T09:04:50Z

Makefile

+# Vector extension instructions
+ENABLE_EXT_V ?= 0
+$(call set-feature, EXT_V)
+VLEN ?= 128 # Default VLEN is 128


Shall this moved into the conditional block of ifeq ($(call has, EXT_V), 1) ?

You're right. I'll move VLEN ?= 128 into the conditional block. Thanks for pointing that out.

ChinYikMing · 2025-02-18T09:32:25Z

src/rv32_template.c

+        }                                                \
+    }
+
+#define VI_LOOP(des, op1, op2, op, SHIFT, MASK, i, j, itr, vm)                 \


The MASK parameter has same name with MASK() function-like macro. I am concern with the naming scheme. Can you pass MASK_BIT as the parameter and leverage the MASK() function-like macro?

P.S. The compiler should smart enough to do constant propagation for the MASK() function-like macro during compile time.

I'll rename MASK to MASK_BIT. Thanks for your suggestion and the information!

jserv · 2025-02-21T02:17:38Z

Can you provide tools to validate RVV compliance? As I know, there are few projects below:

vestata · 2025-02-21T05:55:13Z

@jserv I am using RISC-V Vector Tests Generator to validate the implemented rvv instructions.
Following build-and-test.yml to generate binary test cases with VLEN=128 and XLEN=32.

make all --environment-overrides VLEN=128 XLEN=32

By using riscv32-unknown-elf-objdump to check the binaries in riscv-vector-tests/out/v128x32machine/bin/stage2/ we can see the following pass and fail code sections in every test case.

8001eed4 <fail>:
8001eed4:	0ff0000f          	fence
8001eed8:	00018063          	beqz	gp,8001eed8 <fail+0x4>
8001eedc:	0186                	slli	gp,gp,0x1
8001eede:	0011e193          	ori	gp,gp,1
8001eee2:	05d00893          	li	a7,93
8001eee6:	850e                	mv	a0,gp
8001eee8:	00000073          	ecall

8001eeec <pass>:
8001eeec:	0ff0000f          	fence
8001eef0:	4185                	li	gp,1
8001eef2:	05d00893          	li	a7,93
8001eef6:	4501                	li	a0,0
8001eef8:	00000073          	ecall

I ran the tests using build/rv32emu -t and check the trace output. If the implementation is correct, the trace log includes a pass entry, false on the other hand. I simply wrote a Python script to go though the directory,and so far, 94/1412 test cases passed.

jserv reviewed Jan 24, 2025

View reviewed changes

jserv mentioned this pull request Jan 24, 2025

Add decoder for RVV instructions #501

Closed

jserv requested review from howjmay and vacantron January 24, 2025 15:56

jserv added this to the release-2025.1 milestone Jan 24, 2025

jserv reviewed Jan 24, 2025

View reviewed changes

src/decode.c Outdated Show resolved Hide resolved

jserv reviewed Jan 24, 2025

View reviewed changes

src/decode.c Outdated Show resolved Hide resolved

jserv reviewed Jan 24, 2025

View reviewed changes

jserv requested review from RinHizakura, visitorckw, Risheng1128, ChinYikMing and eleanorLYJ January 24, 2025 16:00

This comment was marked as resolved.

Sign in to view

jserv changed the title ~~Add RVV extension support~~ Support partial vector extension instructions Jan 24, 2025

ChinYikMing reviewed Jan 24, 2025

View reviewed changes

src/riscv.h Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

eleanorLYJ reviewed Jan 25, 2025

View reviewed changes

src/riscv_private.h Outdated Show resolved Hide resolved

eleanorLYJ reviewed Jan 25, 2025

View reviewed changes

src/emulate.c Outdated Show resolved Hide resolved

visitorckw reviewed Jan 25, 2025

View reviewed changes

src/rv32_template.c Outdated Show resolved Hide resolved

visitorckw reviewed Jan 25, 2025

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

visitorckw reviewed Jan 25, 2025

View reviewed changes

src/decode.c Outdated Show resolved Hide resolved

ChinYikMing reviewed Jan 26, 2025

View reviewed changes

src/decode.h Outdated Show resolved Hide resolved

src/decode.c Outdated Show resolved Hide resolved

src/decode.h Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

howjmay reviewed Jan 27, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

bito-code-review bot reviewed Feb 12, 2025

View reviewed changes

src/decode.c Show resolved Hide resolved

bito-code-review bot reviewed Feb 12, 2025

View reviewed changes

This comment was marked as resolved.

Sign in to view

vestata force-pushed the vector branch from 7e23801 to 464c22c Compare February 13, 2025 17:38

bito-code-review bot reviewed Feb 13, 2025

View reviewed changes

src/rv32_constopt.c Show resolved Hide resolved

bito-code-review bot reviewed Feb 13, 2025

View reviewed changes

src/rv32_template.c Outdated Show resolved Hide resolved

bito-code-review bot reviewed Feb 13, 2025

View reviewed changes

sysprog21 deleted a comment from bito-code-review bot Feb 13, 2025

howjmay reviewed Feb 16, 2025

View reviewed changes

jserv reviewed Feb 17, 2025

View reviewed changes

src/rv32_template.c Show resolved Hide resolved

jserv reviewed Feb 17, 2025

View reviewed changes

src/rv32_template.c Outdated Show resolved Hide resolved

jserv reviewed Feb 17, 2025

View reviewed changes

src/rv32_template.c Outdated Show resolved Hide resolved

jserv reviewed Feb 17, 2025

View reviewed changes

src/rv32_template.c Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

vestata added 4 commits February 18, 2025 09:39

vestata force-pushed the vector branch from 464c22c to c3374ea Compare February 18, 2025 02:11

jserv requested review from visitorckw, howjmay and ChinYikMing February 18, 2025 03:20

ChinYikMing reviewed Feb 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support partial vector extension instructions #545

Support partial vector extension instructions #545

vestata commented Jan 24, 2025 •

edited by bito-code-review bot

Loading

jserv left a comment •

edited

Loading

jserv Jan 24, 2025

vestata Jan 25, 2025

This comment was marked as resolved.

vacantron commented Jan 24, 2025

vestata commented Jan 25, 2025

This comment was marked as resolved.

eleanorLYJ commented Jan 25, 2025

vestata commented Jan 26, 2025

This comment was marked as resolved.

howjmay Jan 27, 2025 •

edited

Loading

vestata Jan 27, 2025

This comment was marked as resolved.

bito-code-review bot Feb 12, 2025

This comment was marked as resolved.

bito-code-review bot commented Feb 13, 2025 •

edited

Loading

Code Review Agent Run #0be539

bito-code-review bot Feb 13, 2025

howjmay Feb 16, 2025

vestata Feb 17, 2025

howjmay Feb 16, 2025

jserv Feb 17, 2025

vestata Feb 17, 2025

vestata Feb 17, 2025

howjmay commented Feb 16, 2025 •

edited

Loading

This comment was marked as resolved.

visitorckw commented Feb 18, 2025

ChinYikMing Feb 18, 2025

vestata Feb 18, 2025

ChinYikMing Feb 18, 2025

vestata Feb 18, 2025

jserv commented Feb 21, 2025

vestata commented Feb 21, 2025

-    switch (insn & 0x80000000) {
+/* Mask for bit 31 of vector configuration instructions */
+#define VSETVLI_MASK 0x80000000
+    switch (insn & VSETVLI_MASK) {


		/* clang-format off */

		#define OPT(des, op1, op2, op, op_type) { \

Support partial vector extension instructions #545

Are you sure you want to change the base?

Support partial vector extension instructions #545

Conversation

vestata commented Jan 24, 2025 • edited by bito-code-review bot Loading

Summary by Bito

jserv left a comment • edited Loading

Choose a reason for hiding this comment

Benchmarks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

vacantron commented Jan 24, 2025

vestata commented Jan 25, 2025

This comment was marked as resolved.

eleanorLYJ commented Jan 25, 2025

vestata commented Jan 26, 2025

This comment was marked as resolved.

howjmay Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

bito-code-review bot Feb 12, 2025

Choose a reason for hiding this comment

This comment was marked as resolved.

bito-code-review bot commented Feb 13, 2025 • edited Loading

Code Review Agent Run #0be539

bito-code-review bot Feb 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howjmay commented Feb 16, 2025 • edited Loading

This comment was marked as resolved.

visitorckw commented Feb 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jserv commented Feb 21, 2025

vestata commented Feb 21, 2025

vestata commented Jan 24, 2025 •

edited by bito-code-review bot

Loading

jserv left a comment •

edited

Loading

howjmay Jan 27, 2025 •

edited

Loading

bito-code-review bot commented Feb 13, 2025 •

edited

Loading

howjmay commented Feb 16, 2025 •

edited

Loading