erasure_code: add prefetch and vsetvli optimization by lvshuo2016 · Pull Request #351 · intel/isa-l

lvshuo2016 · 2025-07-29T12:55:10Z

Add prefetch and vsetvli support, on some RV real hardware platforms, its performance can be improved by 100%.

pablodelara · 2025-07-29T14:43:12Z

configure.ac

 		if test "x$rvv" = "xyes"; then
-		  CFLAGS+=" -march=rv64gcv"
-		  CCASFLAGS+=" -march=rv64gcv"
+		  CFLAGS+=" -march=rv64gcv_zicbop"


It looks like this is going to be in conflict with CFLAGS+=" -march=rv64gcv_zbc_zvbc_zvbb" in https://github.com/intel/isa-l/pull/350/files.

They shouldn't conflict because they are on different branches.

I know, but what I meant is that the flags are different. Which one will stay? Or will it be a combination of both? I don't quite understand RISC-V flags

I do a simple compiler test, -march=rv64gcv_zicbop_zbc_zvbc_zvbb,the flags combine both. It compiler successfully.

It should include detection of zicbop at both compile time and runtime, rather than adding it directly.

pablodelara · 2025-07-31T10:58:38Z

Can you sign off the commit? Thanks!

lvshuo2016 · 2025-07-31T12:47:10Z

Can you sign off the commit? Thanks!

Updated. Thanks.

sunyuechi · 2025-07-31T15:22:47Z

Similarly, https://github.com/ChristopherHX/github-act-runner/ encountered a panic and is currently under investigation...

lvshuo2016 · 2025-08-08T03:18:21Z

do you have comments? If have i can fix it.

sunyuechi · 2025-08-08T10:29:13Z

configure.ac

 		if test "x$rvv" = "xyes"; then
-		  CFLAGS+=" -march=rv64gcv"
-		  CCASFLAGS+=" -march=rv64gcv"
+		  CFLAGS+=" -march=rv64gcv_zicbop"


It should include detection of zicbop at both compile time and runtime, rather than adding it directly.

sunyuechi · 2025-08-08T10:29:41Z

erasure_code/riscv64/gf_2vect_dot_prod_rvv.S


-    vsetvli a5, x0, e8, m1  /* Set vector length to maximum */
-
+    vsetvli a5, x0, e8, m1,ta,ma  /* Set vector length to maximum */


Please split the commits — the addition of ta and ma for vsetvli should be in a separate commit.

Why do ta and ma for vsetvli need to be committed separately?

The prefetch part and filling in the default ta and ma for vset are two unrelated matters, and they should generally not be combined in a single commit, as doing so makes the commit appear confusing.

Vsetvli is just a one-line modification, and the function is very clear. Is it necessary to make a separate commit?

The purpose of splitting is to make things simpler — as long as the functionalities are separate, this applies whether it’s a single line or a single word.
Such split commits are actually a necessary requirement in most open-source projects. However, for isa-l specifically, it might be better to let pablodelara decide. I don’t want to say it must be done in a certain way.

I agree with @sunyuechi, even if it is one line. And especially if it is only one line, it should be easy to split into two commits.

pablodelara · 2025-08-25T08:43:26Z

configure.ac

+					".insn i 0x0F, 0, x0, x0, 0x010" ::: "memory"
+				);
+			])],
+			[AC_DEFINE([HAVE_ZICBOP], [1], [Enable Zicbop instructions])


What are the differences in code path if this instruction is available or not?

It should add macro HAVE_ZICBOP judgment conditions in the code.

sunyuechi · 2025-08-26T02:19:06Z

Add prefetch and vsetvli support, on some RV real hardware platforms, its performance can be improved by 100%.

Specifically, which hardware is it?

sunyuechi · 2025-08-27T03:14:15Z

The commits need to be reorganized;

roughly three commits are needed: prefetch, vsetvli default value,
and implementation.

For example, this change and other unrelated logic changes should be moved out of the prefetch commit:

- slli t6, x_vec, 5
+ slli t6, x_vec, 2

The HAVE_ZICBOP macro should be merged into the prefetch commit.

lvshuo2016 · 2025-08-28T08:49:58Z

The commits need to be reorganized;

roughly three commits are needed: prefetch, vsetvli default value, and implementation.

For example, this change and other unrelated logic changes should be moved out of the prefetch commit:
- slli t6, x_vec, 5
+ slli t6, x_vec, 2
The HAVE_ZICBOP macro should be merged into the prefetch commit.

Updated.

lvshuo2016 · 2025-09-03T01:51:00Z

Could this PR be merged?

sunyuechi · 2025-09-03T12:58:08Z

configure.ac

+			[AC_DEFINE([HAVE_ZICBOP], [0], [Disable Zicbop instructions])
+			AM_CONDITIONAL([HAVE_ZICBOP], [false]) zicbop=no]
+		)
+		AC_MSG_RESULT([$zicbop])


This should belong to the third commit, but it's currently in the first commit.

It is currently in third commit. not in first commit.

It is currently in third commit. not in first commit.

+ AC_MSG_RESULT([$zicbop])

is in erasure_code: add optimization implementation, but I think it should be in erasure_code: add prefetch optimization.

It is currently in erasure_code: add prefetch optimization.

commit eac4d0f (HEAD -> riscv-optimize, origin/riscv-optimize)
Author: lvshuo lv.shuo@sanechips.com.cn
Date: Thu Aug 28 16:48:04 2025 +0800

erasure_code: add prefetch optimaztion Signed-off-by: Shuo Lv <lv.shuo@sanechips.com.cn>

diff --git a/configure.ac b/configure.ac
index 1a1476a..e89d660 100644
--- a/configure.ac
+++ b/configure.ac
@@ -38,6 +38,7 @@ AM_CONDITIONAL([CPU_PPC64LE], [test "$CPU" = "ppc64le"])
AM_CONDITIONAL([CPU_RISCV64], [test "$CPU" = "riscv64"])
AM_CONDITIONAL([CPU_UNDEFINED], [test "x$CPU" = "x"])
AM_CONDITIONAL([HAVE_RVV], [false])
+AM_CONDITIONAL([HAVE_ZICBOP], [false])

Check for programs

AC_PROG_CC_STDC
@@ -70,10 +71,28 @@ case "${CPU}" in
[AC_DEFINE([HAVE_RVV], [0], [Disable RVV instructions])
AM_CONDITIONAL([HAVE_RVV], [false]) rvv=no]
)

AC_MSG_CHECKING([Zicbop support])

AC_COMPILE_IFELSE(

[AC_LANG_PROGRAM([], [

__asm__ volatile(

".option arch, +zicbop\n"

".insn i 0x0F, 0, x0, x0, 0x010" ::: "memory"

);

])],

[AC_DEFINE([HAVE_ZICBOP], [1], [Enable Zicbop instructions])

AM_CONDITIONAL([HAVE_ZICBOP], [true]) zicbop=yes],

[AC_DEFINE([HAVE_ZICBOP], [0], [Disable Zicbop instructions])

AM_CONDITIONAL([HAVE_ZICBOP], [false]) zicbop=no]

) AC_MSG_RESULT([$zicbop])

It looks that the picture could not display here.

I used

gh pr checkout 351 git show bbbbf6c

and saw that it’s still in this commit.

It seems there is a problem, my local rebase has a conflict

sunyuechi · 2025-09-03T12:59:10Z

erasure_code/riscv64/gf_2vect_dot_prod_rvv.S


-    vsetvli a5, x0, e8, m1  /* Set vector length to maximum */
-
+    vsetvli a5, x0, e8, m1,ta,ma  /* Set vector length to maximum */


The newly added ta and ma in vset should use the same spacing as in the first half of the line.

sunyuechi · 2025-09-03T13:00:31Z

erasure_code/riscv64/gf_3vect_dot_prod_rvv.S

-    slli t_offset, x_vec, 5
+    slli x_vec, x_vec, 3
+
+    slli t_offset, x_vec, 2


The commit message should include the reason for changing the implementation.

lvshuo2016 · 2025-09-04T08:28:48Z

All is update.

sunyuechi · 2025-09-04T08:42:53Z

Add prefetch and vsetvli support, on some RV real hardware platforms, its performance can be improved by 100%.

Specifically, which hardware is it?

internal self-developed chip, but it has no effect on the current development board which is the sequentially launched core

Please try to explain the situation and/or describe the test environment in the commit message.

lvshuo2016 · 2025-09-05T07:00:59Z

All is updated.

lvshuo2016 · 2025-09-08T00:50:49Z

Could this PR be merged?

pablodelara · 2025-09-10T09:11:38Z

@sunyuechi what do you think of the latest code?

pablodelara · 2025-09-10T09:20:13Z

@lvshuo2016 one more comment. Could you review your commit messages? There are typos and also the "Signed-off-by" line should be at the end of message always.

pablodelara · 2025-09-17T22:42:25Z

I think it’s reasonable to either wait until hwprobe is merged before adding prefetch, or to first add prefetch behind a compile-time option (default off) as an experimental feature.

If it’s to be merged now, there are some adjustments needed:

Currently there are cases where the compiler supports it but the CPU does not, so it’s necessary to change the build to ensure prefetch is also disabled by default even if the compiler supports it.

Since there is no publicly available hardware for validation, the commit message for prefetch should include an explanation (how it was tested, what effect it has, and why prefetch is being added, Including an explanation of the current situation of this extension detection.).

When you say wait until hwprobe is merged, do you mean https://lkml.org/lkml/2025/9/11/849?
Being this an external dependency, it's going to make this tough...

sunyuechi · 2025-09-18T02:17:05Z

@pablodelara Yes, I’m okay with either merging quickly or waiting until hwprobe is merged — it mainly depends on how the author decides to adjust the patch.

pablodelara · 2025-09-18T14:50:56Z

@pablodelara Yes, I’m okay with either merging quickly or waiting until hwprobe is merged — it mainly depends on how the author decides to adjust the patch.

Up to you both. I'm OK merging it if you are OK with it, with the changes required.

lvshuo2016 · 2025-09-19T02:56:13Z

I think it’s reasonable to either wait until hwprobe is merged before adding prefetch, or to first add prefetch behind a compile-time option (default off) as an experimental feature.
If it’s to be merged now, there are some adjustments needed:

Currently there are cases where the compiler supports it but the CPU does not, so it’s necessary to change the build to ensure prefetch is also disabled by default even if the compiler supports it.

Since there is no publicly available hardware for validation, the commit message for prefetch should include an explanation (how it was tested, what effect it has, and why prefetch is being added, Including an explanation of the current situation of this extension detection.).

When you say wait until hwprobe is merged, do you mean https://lkml.org/lkml/2025/9/11/849? Being this an external dependency, it's going to make this tough...

I remove the prefetch patch,the current code could be merge. I will create a new pull request to track prefetch(include runtime check).

sunyuechi · 2025-09-21T12:51:19Z

erasure_code: add optimization implementation

The title contains an extra space.

erasure_code: set vsetvli to default parameter

add vsetvli parameter to default values

Signed-off-by: Shuo Lv <lv.shuo@sanechips.com.cn>

The commit message is redundant with the title; you can keep only the title.

erasure_code/riscv64/gf_7vect_dot_prod_rvv.S

In gf[2-6], slli was modified, but not in 7. Is there a reason for this, or was it an oversight?

sunyuechi · 2025-09-21T12:53:04Z

erasure_code/riscv64/gf_2vect_dot_prod_rvv.S


+    /* calc for next */
+    addi x_vec_i, x_vec_i, 8   /* move x_vec_i to next */
+    add a6,x_src,x_vec_i


As with the other parts, please add a space after the comma (the other files also have this instruction followed by a space).

Please also check other parts, for example in this file the line add x_ptr,x_ptr,x_pos still has this formatting issue.

Please also check other parts, for example in this file the line add x_ptr,x_ptr,x_pos still has this formatting issue.

Here the code which you see is old code. I have updated all related formatting issue.

I used “gh pr checkout 351” to download the latest code, but line 102 is still like this.

I used “gh pr checkout 351” to download the latest code, but line 102 is still like this.

It is updated in commit fd94836.

It is updated in commit fd94836.

It was updated in commit fd94836.

m your prev

Updated.

You can use this command grep -rPn '\,\S' erasure_code/riscv64 to find such formatting issues. It looks like there are still more than 30 places missing a space.

./gf_7vect_dot_prod_rvv.S:163: add x_ptr,x_ptr,x_pos ./gf_7vect_dot_prod_rvv.S:262: add x_dest1,x_dest1, t0 ./gf_7vect_dot_prod_rvv.S:263: add x_dest2,x_dest2, t0 ./gf_7vect_dot_prod_rvv.S:264: add x_dest3,x_dest3, t0 ./gf_7vect_dot_prod_rvv.S:265: add x_dest4,x_dest4, t0 ./gf_7vect_dot_prod_rvv.S:266: add x_dest5,x_dest5, t0 ./gf_7vect_dot_prod_rvv.S:267: add x_dest6,x_dest6, t0 ./gf_7vect_dot_prod_rvv.S:268: add x_dest7,x_dest7, t0 ./gf_4vect_dot_prod_rvv.S:130: add x_ptr,x_ptr,x_pos ...

lvshuo2016 · 2025-09-22T03:25:39Z

erasure_code: add optimization implementation

The title contains an extra space.
erasure_code: set vsetvli to default parameter

add vsetvli parameter to default values

Signed-off-by: Shuo Lv <lv.shuo@sanechips.com.cn>
The commit message is redundant with the title; you can keep only the title.

erasure_code/riscv64/gf_7vect_dot_prod_rvv.S

In gf[2-6], slli was modified, but not in 7. Is there a reason for this, or was it an oversight?

Updated. In gf[2-6], slli was modified, but not in 7. Here +7 only for test, m+7 is not used in actual scenarios. So it is not modified.

sunyuechi · 2025-09-22T06:21:46Z

Is gf5 the same case? From a code quality perspective, it might be better to modify gf5 and gf7 to use a consistent implementation, so that the test code logic is aligned with the production code. Otherwise, if they are not modified, at least the commit message should explain why they remain inconsistent.

reduce one slli instructions and remove the dependence between vle8.v and ld instructions gf5 and gf7 are not modified, +5 and +7 are not used in actual scenarios. Signed-off-by: Shuo Lv <lv.shuo@sanechips.com.cn>

lvshuo2016 · 2025-09-25T02:56:14Z

Is gf5 the same case? From a code quality perspective, it might be better to modify gf5 and gf7 to use a consistent implementation, so that the test code logic is aligned with the production code. Otherwise, if they are not modified, at least the commit message should explain why they remain inconsistent.

the commit message is added.

lvshuo2016 · 2025-09-25T02:57:40Z

It looks like an environmental issue for the last build failure.
The self-hosted runner lost communication with the server. Verify the machine is running and has a healthy network connection. Anything in your workflow that terminates the runner process, starves it for CPU/Memory, or blocks its network access can cause this error.

lvshuo2016 · 2025-09-26T08:19:47Z

All is updated. Could it be merged now?

sunyuechi · 2025-09-28T08:18:55Z

erasure_code/riscv64/gf_7vect_dot_prod_rvv.S

    vle8.v v_gft7_hi, (x_tbl7)
    addi x_tbl7, x_tbl7, 16

-


You can also remove other consecutive blank lines, for example:

gf_3vect_dot_prod_rvv.S:60 gf_5vect_dot_prod_rvv.S:206 gf_6vect_dot_prod_rvv.S:153 gf_7vect_dot_prod_rvv.S:97 gf_7vect_dot_prod_rvv.S:244 gf_vect_dot_prod_rvv.S:30

lvshuo2016 · 2025-09-28T14:01:20Z

ALL is updated.

sunyuechi · 2025-09-29T09:14:02Z

LGTM

pablodelara · 2025-10-02T10:24:51Z

erasure_code/riscv64/gf_2vect_dot_prod_rvv.S

@@ -92,16 +93,13 @@ gf_2vect_dot_prod_rvv:

    /* gf_tbl base = (x_tbl + dest_idx * x_vec * 32) */


Should this 32 be 4 now, since you are shifting left 2 instead of 5 bits?

Totally it need shift left 5, slli x_vec, x_vec, 3 + slli t6, x_vec, 2. You are right, here 32 in comments should be 4, we could add extra comments.

Signed-off-by: Shuo Lv <lv.shuo@sanechips.com.cn>

lvshuo2016 · 2025-10-11T01:00:51Z

Could it be merged now?

pablodelara · 2025-10-13T09:01:49Z

Could it be merged now?

We will merge this PR shortly, thanks.

pablodelara · 2025-10-13T09:17:36Z

This is merged now, thanks.

pablodelara reviewed Jul 29, 2025

View reviewed changes

lvshuo2016 force-pushed the riscv-optimize branch from 3b900a8 to 54339a6 Compare July 31, 2025 12:45

lvshuo2016 force-pushed the riscv-optimize branch from 54339a6 to 41fa113 Compare August 6, 2025 10:04

sunyuechi reviewed Aug 8, 2025

View reviewed changes

lvshuo2016 force-pushed the riscv-optimize branch from 41fa113 to 64e23f0 Compare August 12, 2025 03:00

lvshuo2016 force-pushed the riscv-optimize branch from 64e23f0 to 339b646 Compare August 24, 2025 08:43

pablodelara reviewed Aug 25, 2025

View reviewed changes

lvshuo2016 force-pushed the riscv-optimize branch 3 times, most recently from 87ab99c to be89c37 Compare August 28, 2025 08:38

sunyuechi reviewed Sep 3, 2025

View reviewed changes

lvshuo2016 force-pushed the riscv-optimize branch 2 times, most recently from 6a8fa6c to eac4d0f Compare September 4, 2025 08:27

lvshuo2016 force-pushed the riscv-optimize branch 3 times, most recently from 2f626d5 to 99d5360 Compare September 5, 2025 06:58

lvshuo2016 force-pushed the riscv-optimize branch from c4bbe99 to 2e5b5e1 Compare September 19, 2025 02:54

sunyuechi reviewed Sep 21, 2025

View reviewed changes

lvshuo2016 force-pushed the riscv-optimize branch 3 times, most recently from 210efde to 3d159c3 Compare September 22, 2025 03:22

erasure_code: add optimization implementation

fd94836

reduce one slli instructions and remove the dependence between vle8.v and ld instructions gf5 and gf7 are not modified, +5 and +7 are not used in actual scenarios. Signed-off-by: Shuo Lv <lv.shuo@sanechips.com.cn>

lvshuo2016 force-pushed the riscv-optimize branch from 3d159c3 to 9f09b4a Compare September 25, 2025 02:54

sunyuechi reviewed Sep 28, 2025

View reviewed changes

lvshuo2016 force-pushed the riscv-optimize branch 2 times, most recently from 4d428aa to 33da77d Compare September 28, 2025 13:59

lvshuo2016 force-pushed the riscv-optimize branch from 33da77d to 757020a Compare September 29, 2025 07:41

pablodelara reviewed Oct 2, 2025

View reviewed changes

erasure_code: set vsetvli to default parameter and add space

918e305

Signed-off-by: Shuo Lv <lv.shuo@sanechips.com.cn>

lvshuo2016 force-pushed the riscv-optimize branch from 757020a to 918e305 Compare October 9, 2025 06:13

pablodelara closed this Oct 13, 2025


		vsetvli a5, x0, e8, m1 /* Set vector length to maximum */

		vsetvli a5, x0, e8, m1,ta,ma /* Set vector length to maximum */

		@@ -92,16 +93,13 @@ gf_2vect_dot_prod_rvv:

		/* gf_tbl base = (x_tbl + dest_idx * x_vec * 32) */

Comments

Conversation

lvshuo2016 commented Jul 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pablodelara commented Jul 31, 2025

Uh oh!

lvshuo2016 commented Jul 31, 2025

Uh oh!

sunyuechi commented Jul 31, 2025

Uh oh!

lvshuo2016 commented Aug 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lvshuo2016 Aug 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sunyuechi commented Aug 26, 2025

Uh oh!

sunyuechi commented Aug 27, 2025

Uh oh!

lvshuo2016 commented Aug 28, 2025

Uh oh!

lvshuo2016 commented Sep 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Check for programs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lvshuo2016 Aug 11, 2025 •

edited

Loading

pablodelara commented Sep 10, 2025 •

edited

Loading

lvshuo2016 commented Sep 19, 2025 •

edited

Loading