| Message ID | 20231003022724.2875-1-kito.cheng@sifive.com |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Fix the riscv_legitimize_poly_move issue on targets where the minimal VLEN exceeds 512. | expand |
I think the "max poly value" is the LMUL 1 mode coeffs[1]
See int vlenb = BYTES_PER_RISCV_VECTOR.coeffs[1];
So I think bump max_power to exact_log2 (64); is not enough.
since we adjust the LMUL 1 mode size according to TARGET_MIN_VLEN.
I suspect the testcase you append in this patch will fail with -march=rv64gcv_zvl4096b.
I think it's more reasonable bump the max power into BYTES_PER_RISCV_VECTOR.coeffs[1]
It should be more reasonable:
+ int max_power = exact_log2 (64);
change it into
+ int max_power = exact_log2 (BYTES_PER_RISCV_VECTOR.coeffs[0]);
juzhe.zhong@rivai.ai
From: Kito Cheng
Date: 2023-10-03 10:27
To: gcc-patches; kito.cheng; palmer; jeffreyalaw; rdapp; juzhe.zhong
CC: Kito Cheng
Subject: [PATCH] RISC-V: Fix the riscv_legitimize_poly_move issue on targets where the minimal VLEN exceeds 512.
riscv_legitimize_poly_move was expected to ensure the poly value is at most 32
times smaller than the minimal VLEN (32 being derived from '4096 / 128').
This assumption held when our mode modeling was not so precisely defined.
However, now that we have modeled the mode size according to the correct minimal
VLEN info, the size difference between different RVV modes can be up to 64
times. For instance, comparing RVVMF64BI and RVVMF1BI, the sizes are [1, 1]
versus [64, 64] respectively.
gcc/ChangeLog:
* config/riscv/riscv.cc (riscv_legitimize_poly_move): Bump
max_power to 64.
gcc/testsuite/ChangeLog:
* g++.target/riscv/rvv/autovec/bug-01.C: New.
* g++.target/riscv/rvv/rvv.exp: Add autovec folder.
---
gcc/config/riscv/riscv.cc | 7 ++--
.../g++.target/riscv/rvv/autovec/bug-01.C | 33 +++++++++++++++++++
gcc/testsuite/g++.target/riscv/rvv/rvv.exp | 3 ++
3 files changed, 40 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/g++.target/riscv/rvv/autovec/bug-01.C
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index d5446b63dbf..6468702e3a3 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -2386,9 +2386,10 @@ riscv_legitimize_poly_move (machine_mode mode, rtx dest, rtx tmp, rtx src)
}
else
{
- /* FIXME: We currently DON'T support TARGET_MIN_VLEN > 4096. */
- int max_power = exact_log2 (4096 / 128);
- for (int i = 0; i < max_power; i++)
+ /* The size difference between different RVV modes can be up to 64 times.
+ e.g. RVVMF64BI vs RVVMF1BI on zvl512b, which is [1, 1] vs [64, 64]. */
+ int max_power = exact_log2 (64);
+ for (int i = 0; i <= max_power; i++)
{
int possible_div_factor = 1 << i;
if (factor % (vlenb / possible_div_factor) == 0)
diff --git a/gcc/testsuite/g++.target/riscv/rvv/autovec/bug-01.C b/gcc/testsuite/g++.target/riscv/rvv/autovec/bug-01.C
new file mode 100644
index 00000000000..fd10009ddbe
--- /dev/null
+++ b/gcc/testsuite/g++.target/riscv/rvv/autovec/bug-01.C
@@ -0,0 +1,33 @@
+/* { dg-options "-march=rv64gcv_zvl512b -mabi=lp64d -O3" } */
+
+class c {
+public:
+ int e();
+ void j();
+};
+float *d;
+class k {
+ int f;
+
+public:
+ k(int m) : f(m) {}
+ float g;
+ float h;
+ void n(int m) {
+ for (int i; i < m; i++) {
+ d[0] = d[1] = d[2] = g;
+ d[3] = h;
+ d += f;
+ }
+ }
+};
+c l;
+void o() {
+ int b = l.e();
+ k a(b);
+ for (;;)
+ if (b == 4) {
+ l.j();
+ a.n(2);
+ }
+}
diff --git a/gcc/testsuite/g++.target/riscv/rvv/rvv.exp b/gcc/testsuite/g++.target/riscv/rvv/rvv.exp
index 249530580d7..c30d6e93144 100644
--- a/gcc/testsuite/g++.target/riscv/rvv/rvv.exp
+++ b/gcc/testsuite/g++.target/riscv/rvv/rvv.exp
@@ -40,5 +40,8 @@ set CFLAGS "-march=$gcc_march -O3"
dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/base/*.C]] \
"" $CFLAGS
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/autovec/*.\[C\]]] \
+ "" $CFLAGS
+
# All done.
dg-finish
钟居哲 <juzhe.zhong@rivai.ai>於 2023年10月4日 週三，20:20寫道： > > I think the "max poly value" is the LMUL 1 mode coeffs[1] > > See int vlenb = BYTES_PER_RISCV_VECTOR.coeffs[1]; > > So I think bump max_power to exact_log2 (64); is not enough. > since we adjust the LMUL 1 mode size according to TARGET_MIN_VLEN. > > I suspect the testcase you append in this patch will fail with -march=rv64gcv_zvl4096b. There is no type smaller than [64, 64] in zvl4096b, RVVMF64BI is [64, 64], it’s smallest type, and RVVFM1BI is [512, 512] (size of single vector reg.) which at most 64x for zvl4096b, so my understanding is log2(64) is enough :) and of cause, verified the testcase is work with -march=rv64gcv_zvl4096b
OK. But could you add a MACRO define Something like: #define MAX_POLY_VARIANT 64 juzhe.zhong@rivai.ai From: Kito Cheng Date: 2023-10-04 21:32 To: 钟居哲 CC: Jeff Law; gcc-patches; kito.cheng; palmer; rdapp Subject: Re: [PATCH] RISC-V: Fix the riscv_legitimize_poly_move issue on targets where the minimal VLEN exceeds 512. 钟居哲 <juzhe.zhong@rivai.ai>於 2023年10月4日 週三，20:20寫道： > > I think the "max poly value" is the LMUL 1 mode coeffs[1] > > See int vlenb = BYTES_PER_RISCV_VECTOR.coeffs[1]; > > So I think bump max_power to exact_log2 (64); is not enough. > since we adjust the LMUL 1 mode size according to TARGET_MIN_VLEN. > > I suspect the testcase you append in this patch will fail with -march=rv64gcv_zvl4096b. There is no type smaller than [64, 64] in zvl4096b, RVVMF64BI is [64, 64], it’s smallest type, and RVVFM1BI is [512, 512] (size of single vector reg.) which at most 64x for zvl4096b, so my understanding is log2(64) is enough :) and of cause, verified the testcase is work with -march=rv64gcv_zvl4096b
>> I think the "max poly value" is the LMUL 1 mode coeffs[1] >> >> See int vlenb = BYTES_PER_RISCV_VECTOR.coeffs[1]; >> >> So I think bump max_power to exact_log2 (64); is not enough. >> since we adjust the LMUL 1 mode size according to TARGET_MIN_VLEN. >> >> I suspect the testcase you append in this patch will fail with -march=rv64gcv_zvl4096b. > > > There is no type smaller than [64, 64] in zvl4096b, RVVMF64BI is [64, > 64], it’s smallest type, and RVVFM1BI is [512, 512] (size of single > vector reg.) which at most 64x for zvl4096b, so my understanding is > log2(64) is enough :) > > and of cause, verified the testcase is work with -march=rv64gcv_zvl4096b I was wondering if the whole hunk couldn't be condensed into something like (untested): div_factor = wi::ctz (factor) - wi::ctz (vlenb); if (div_factor >= 0) div_factor = 1; else div_factor = 1 << -div_factor; This would avoid the loop as well. An assert for the div_factor (not exceeding a value) could still be added. Regards Robin
Hi Robin: Your suggested code seems work fine, let me run more test and send v2, I guess I just don’t know how to explain why it work in comment :p Robin Dapp <rdapp.gcc@gmail.com>於 2023年10月5日 週四，03:57寫道： > >> I think the "max poly value" is the LMUL 1 mode coeffs[1] > >> > >> See int vlenb = BYTES_PER_RISCV_VECTOR.coeffs[1]; > >> > >> So I think bump max_power to exact_log2 (64); is not enough. > >> since we adjust the LMUL 1 mode size according to TARGET_MIN_VLEN. > >> > >> I suspect the testcase you append in this patch will fail with > -march=rv64gcv_zvl4096b. > > > > > > There is no type smaller than [64, 64] in zvl4096b, RVVMF64BI is [64, > > 64], it’s smallest type, and RVVFM1BI is [512, 512] (size of single > > vector reg.) which at most 64x for zvl4096b, so my understanding is > > log2(64) is enough :) > > > > and of cause, verified the testcase is work with -march=rv64gcv_zvl4096b > > I was wondering if the whole hunk couldn't be condensed into something > like (untested): > > div_factor = wi::ctz (factor) - wi::ctz (vlenb); > if (div_factor >= 0) > div_factor = 1; > else > div_factor = 1 << -div_factor; > > This would avoid the loop as well. An assert for the div_factor (not > exceeding a value) could still be added. > > Regards > Robin >
> Your suggested code seems work fine, let me run more test and send > v2, I guess I just don’t know how to explain why it work in comment > :p If it's too convoluted maybe we should rather not use it :D The idea is for factor % (vlenb / potential_div) == 0 we're actually looking for the largest power of 2 that factor and vlenb are both a multiple of. (Individually) for factor the largest pow2 can be calculated by ctz (factor). Same for vlenb via ctz (vlenb). If ctz (factor) >= ctz (vlenb), vlenb already divides factor evenly. Otherwise, i.e. when ctz (factor) = 1 (divisible by 2) and ctz (vlenb) = 4 (divisible by 16) we need to divide vlenb by the pow2 difference 1 << (4 - 1). I just realize that the explanation is longer than the code before, maybe not a good sign ;) Regards Robin
Any update of this patch? Currently, we are running vect testsuite with -march=rv64gcv_zvl128b I am planning to run vect testsuite with these following combinations: -march=rv64gcv_zvl128b --param=riscv-autovec-lmul=m1 -march=rv64gcv_zvl128b --param=riscv-autovec-lmul=m2 -march=rv64gcv_zvl128b --param=riscv-autovec-lmul=m4 -march=rv64gcv_zvl128b --param=riscv-autovec-lmul=m8 -march=rv64gcv_zvl128b --param=riscv-autovec-lmul=dynamic -march=rv64gcv_zvl256b --param=riscv-autovec-lmul=m1 -march=rv64gcv_zvl256b --param=riscv-autovec-lmul=m2 -march=rv64gcv_zvl256b --param=riscv-autovec-lmul=m4 -march=rv64gcv_zvl256b --param=riscv-autovec-lmul=m8 -march=rv64gcv_zvl256b --param=riscv-autovec-lmul=dynamic -march=rv64gcv_zvl512b --param=riscv-autovec-lmul=m1 -march=rv64gcv_zvl512b --param=riscv-autovec-lmul=m2 -march=rv64gcv_zvl512b --param=riscv-autovec-lmul=m4 -march=rv64gcv_zvl512b --param=riscv-autovec-lmul=m8 -march=rv64gcv_zvl512b --param=riscv-autovec-lmul=dynamic -march=rv64gcv_zvl1024b --param=riscv-autovec-lmul=m1 -march=rv64gcv_zvl1024b --param=riscv-autovec-lmul=m2 -march=rv64gcv_zvl1024b --param=riscv-autovec-lmul=m4 -march=rv64gcv_zvl1024b --param=riscv-autovec-lmul=m8 -march=rv64gcv_zvl1024b --param=riscv-autovec-lmul=dynamic -march=rv64gcv_zvl2048b --param=riscv-autovec-lmul=m1 -march=rv64gcv_zvl2048b --param=riscv-autovec-lmul=m2 -march=rv64gcv_zvl2048b --param=riscv-autovec-lmul=m4 -march=rv64gcv_zvl2048b --param=riscv-autovec-lmul=m8 -march=rv64gcv_zvl2048b --param=riscv-autovec-lmul=dynamic -march=rv64gcv_zvl4096b --param=riscv-autovec-lmul=m1 -march=rv64gcv_zvl4096b --param=riscv-autovec-lmul=m2 -march=rv64gcv_zvl4096b --param=riscv-autovec-lmul=m4 -march=rv64gcv_zvl4096b --param=riscv-autovec-lmul=m8 -march=rv64gcv_zvl4096b --param=riscv-autovec-lmul=dynamic I will do that in stage 3. I hope this patch can be landed before I do that. juzhe.zhong@rivai.ai From: Kito Cheng Date: 2023-10-05 22:00 To: Robin Dapp CC: Jeff Law; gcc-patches; kito.cheng; palmer; 钟居哲 Subject: Re: [PATCH] RISC-V: Fix the riscv_legitimize_poly_move issue on targets where the minimal VLEN exceeds 512. Hi Robin: Your suggested code seems work fine, let me run more test and send v2, I guess I just don’t know how to explain why it work in comment :p Robin Dapp <rdapp.gcc@gmail.com>於 2023年10月5日 週四，03:57寫道： >> I think the "max poly value" is the LMUL 1 mode coeffs[1] >> >> See int vlenb = BYTES_PER_RISCV_VECTOR.coeffs[1]; >> >> So I think bump max_power to exact_log2 (64); is not enough. >> since we adjust the LMUL 1 mode size according to TARGET_MIN_VLEN. >> >> I suspect the testcase you append in this patch will fail with -march=rv64gcv_zvl4096b. > > > There is no type smaller than [64, 64] in zvl4096b, RVVMF64BI is [64, > 64], it’s smallest type, and RVVFM1BI is [512, 512] (size of single > vector reg.) which at most 64x for zvl4096b, so my understanding is > log2(64) is enough :) > > and of cause, verified the testcase is work with -march=rv64gcv_zvl4096b I was wondering if the whole hunk couldn't be condensed into something like (untested): div_factor = wi::ctz (factor) - wi::ctz (vlenb); if (div_factor >= 0) div_factor = 1; else div_factor = 1 << -div_factor; This would avoid the loop as well. An assert for the div_factor (not exceeding a value) could still be added. Regards Robin
Yeah, I'll take you suggestion and go ahead, Robin's suggestion is great but it's just a little too magic :P On Wed, Oct 11, 2023 at 4:00 PM 钟居哲 <juzhe.zhong@rivai.ai> wrote: > > Any update of this patch? > > Currently, we are running vect testsuite with -march=rv64gcv_zvl128b > > I am planning to run vect testsuite with these following combinations: > > -march=rv64gcv_zvl128b --param=riscv-autovec-lmul=m1 > -march=rv64gcv_zvl128b --param=riscv-autovec-lmul=m2 > -march=rv64gcv_zvl128b --param=riscv-autovec-lmul=m4 > -march=rv64gcv_zvl128b --param=riscv-autovec-lmul=m8 > -march=rv64gcv_zvl128b --param=riscv-autovec-lmul=dynamic > > -march=rv64gcv_zvl256b --param=riscv-autovec-lmul=m1 > -march=rv64gcv_zvl256b --param=riscv-autovec-lmul=m2 > -march=rv64gcv_zvl256b --param=riscv-autovec-lmul=m4 > -march=rv64gcv_zvl256b --param=riscv-autovec-lmul=m8 > -march=rv64gcv_zvl256b --param=riscv-autovec-lmul=dynamic > > -march=rv64gcv_zvl512b --param=riscv-autovec-lmul=m1 > -march=rv64gcv_zvl512b --param=riscv-autovec-lmul=m2 > -march=rv64gcv_zvl512b --param=riscv-autovec-lmul=m4 > -march=rv64gcv_zvl512b --param=riscv-autovec-lmul=m8 > -march=rv64gcv_zvl512b --param=riscv-autovec-lmul=dynamic > > -march=rv64gcv_zvl1024b --param=riscv-autovec-lmul=m1 > -march=rv64gcv_zvl1024b --param=riscv-autovec-lmul=m2 > -march=rv64gcv_zvl1024b --param=riscv-autovec-lmul=m4 > -march=rv64gcv_zvl1024b --param=riscv-autovec-lmul=m8 > -march=rv64gcv_zvl1024b --param=riscv-autovec-lmul=dynamic > > -march=rv64gcv_zvl2048b --param=riscv-autovec-lmul=m1 > -march=rv64gcv_zvl2048b --param=riscv-autovec-lmul=m2 > -march=rv64gcv_zvl2048b --param=riscv-autovec-lmul=m4 > -march=rv64gcv_zvl2048b --param=riscv-autovec-lmul=m8 > -march=rv64gcv_zvl2048b --param=riscv-autovec-lmul=dynamic > > -march=rv64gcv_zvl4096b --param=riscv-autovec-lmul=m1 > -march=rv64gcv_zvl4096b --param=riscv-autovec-lmul=m2 > -march=rv64gcv_zvl4096b --param=riscv-autovec-lmul=m4 > -march=rv64gcv_zvl4096b --param=riscv-autovec-lmul=m8 > -march=rv64gcv_zvl4096b --param=riscv-autovec-lmul=dynamic > > I will do that in stage 3. I hope this patch can be landed before I do that. > ________________________________ > juzhe.zhong@rivai.ai > > > From: Kito Cheng > Date: 2023-10-05 22:00 > To: Robin Dapp > CC: Jeff Law; gcc-patches; kito.cheng; palmer; 钟居哲 > Subject: Re: [PATCH] RISC-V: Fix the riscv_legitimize_poly_move issue on targets where the minimal VLEN exceeds 512. > Hi Robin: > > Your suggested code seems work fine, let me run more test and send v2, I guess I just don’t know how to explain why it work in comment :p > > Robin Dapp <rdapp.gcc@gmail.com>於 2023年10月5日 週四，03:57寫道： >> >> >> I think the "max poly value" is the LMUL 1 mode coeffs[1] >> >> >> >> See int vlenb = BYTES_PER_RISCV_VECTOR.coeffs[1]; >> >> >> >> So I think bump max_power to exact_log2 (64); is not enough. >> >> since we adjust the LMUL 1 mode size according to TARGET_MIN_VLEN. >> >> >> >> I suspect the testcase you append in this patch will fail with -march=rv64gcv_zvl4096b. >> > >> > >> > There is no type smaller than [64, 64] in zvl4096b, RVVMF64BI is [64, >> > 64], it’s smallest type, and RVVFM1BI is [512, 512] (size of single >> > vector reg.) which at most 64x for zvl4096b, so my understanding is >> > log2(64) is enough :) >> > >> > and of cause, verified the testcase is work with -march=rv64gcv_zvl4096b >> >> I was wondering if the whole hunk couldn't be condensed into something >> like (untested): >> >> div_factor = wi::ctz (factor) - wi::ctz (vlenb); >> if (div_factor >= 0) >> div_factor = 1; >> else >> div_factor = 1 << -div_factor; >> >> This would avoid the loop as well. An assert for the div_factor (not >> exceeding a value) could still be added. >> >> Regards >> Robin
On 10/11/23 17:17, Kito Cheng wrote: > Yeah, I'll take you suggestion and go ahead, Robin's suggestion is > great but it's just a little too magic :P So there'll be a V2 of this patch, right? Just want to make sure state is correct in patchwork. jeff
Yeah, will send v2 today Jeff Law <jeffreyalaw@gmail.com> 於 2023年10月12日 週四 09:15 寫道： > > > On 10/11/23 17:17, Kito Cheng wrote: > > Yeah, I'll take you suggestion and go ahead, Robin's suggestion is > > great but it's just a little too magic :P > So there'll be a V2 of this patch, right? Just want to make sure state > is correct in patchwork. > > jeff >
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc index d5446b63dbf..6468702e3a3 100644 --- a/gcc/config/riscv/riscv.cc +++ b/gcc/config/riscv/riscv.cc @@ -2386,9 +2386,10 @@ riscv_legitimize_poly_move (machine_mode mode, rtx dest, rtx tmp, rtx src) } else { - /* FIXME: We currently DON'T support TARGET_MIN_VLEN > 4096. */ - int max_power = exact_log2 (4096 / 128); - for (int i = 0; i < max_power; i++) + /* The size difference between different RVV modes can be up to 64 times. + e.g. RVVMF64BI vs RVVMF1BI on zvl512b, which is [1, 1] vs [64, 64]. */ + int max_power = exact_log2 (64); + for (int i = 0; i <= max_power; i++) { int possible_div_factor = 1 << i; if (factor % (vlenb / possible_div_factor) == 0) diff --git a/gcc/testsuite/g++.target/riscv/rvv/autovec/bug-01.C b/gcc/testsuite/g++.target/riscv/rvv/autovec/bug-01.C new file mode 100644 index 00000000000..fd10009ddbe --- /dev/null +++ b/gcc/testsuite/g++.target/riscv/rvv/autovec/bug-01.C @@ -0,0 +1,33 @@ +/* { dg-options "-march=rv64gcv_zvl512b -mabi=lp64d -O3" } */ + +class c { +public: + int e(); + void j(); +}; +float *d; +class k { + int f; + +public: + k(int m) : f(m) {} + float g; + float h; + void n(int m) { + for (int i; i < m; i++) { + d[0] = d[1] = d[2] = g; + d[3] = h; + d += f; + } + } +}; +c l; +void o() { + int b = l.e(); + k a(b); + for (;;) + if (b == 4) { + l.j(); + a.n(2); + } +} diff --git a/gcc/testsuite/g++.target/riscv/rvv/rvv.exp b/gcc/testsuite/g++.target/riscv/rvv/rvv.exp index 249530580d7..c30d6e93144 100644 --- a/gcc/testsuite/g++.target/riscv/rvv/rvv.exp +++ b/gcc/testsuite/g++.target/riscv/rvv/rvv.exp @@ -40,5 +40,8 @@ set CFLAGS "-march=$gcc_march -O3" dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/base/*.C]] \ "" $CFLAGS +dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/autovec/*.\[C\]]] \ + "" $CFLAGS + # All done. dg-finish