| Message ID | 20240810123636.3620592-1-pan2.li@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] RISC-V: Bugfix incorrect operand for vwsll auto-vect | expand |
A bit of bikeshedding: While it's obviously a bug, I'm not really sure it's useful to truncate before emitting the widening shift. Do we save an instruction vs. the regular non-widening shift by doing so? I think my original (failed) idea was this pattern to be an intermediate/bridge pattern that never splits. Once we need to "split" maybe the regular shift is better or at least similar?
> I think my original (failed) idea was this pattern to be an intermediate/bridge > pattern that never splits. Yes, this pattern should not be hit by design, and any changes to the layout of pattern may result in some vwsll autovec failure. > Once we need to "split" maybe the regular shift is > better or at least similar? Actually it is something similar to short = char << int. Maybe we can 1. extend char to short. 2. truncate int to short. Then regular short shift is suitable here. Honestly I am not sure it is better than vwsll. Pan -----Original Message----- From: Robin Dapp <rdapp.gcc@gmail.com> Sent: Saturday, August 10, 2024 10:32 PM To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; jeffreyalaw@gmail.com; Robin Dapp <rdapp.gcc@gmail.com> Subject: Re: [PATCH v1] RISC-V: Bugfix incorrect operand for vwsll auto-vect A bit of bikeshedding: While it's obviously a bug, I'm not really sure it's useful to truncate before emitting the widening shift. Do we save an instruction vs. the regular non-widening shift by doing so? I think my original (failed) idea was this pattern to be an intermediate/bridge pattern that never splits. Once we need to "split" maybe the regular shift is better or at least similar?
On 8/10/24 8:31 AM, Robin Dapp wrote: > A bit of bikeshedding: > > While it's obviously a bug, I'm not really sure it's useful to truncate before > emitting the widening shift. Do we save an instruction vs. the regular > non-widening shift by doing so? At least for the test you added, there is no different before/after Pan's patch. If we disable the pattern entirely, then yes it gets notably worse -- instead of a single vwsll we end up two extensions, the vsll, and a vset+vncvt for the final conversion. So yea, the pattern still seems to be worth having :-0 Jeff
On 8/10/24 6:36 AM, pan2.li@intel.com wrote: > This patch would like to fix one ICE when rv64gcv_zvbb for vwsll. > Consider below example. > > void vwsll_vv_test (short *restrict dst, char *restrict a, > int *restrict b, int n) > { > for (int i = 0; i < n; i++) > dst[i] = a[i] << b[i]; > } > > It will hit the vwsll pattern with following operands. > operand 0 -> (reg:RVVMF2HI 146 [ vect__7.13 ]) > operand 1 -> (reg:RVVMF4QI 165 [ vect_cst__33 ]) > operand 2 -> (reg:RVVM1SI 171 [ vect_cst__36 ]) > > According to the ISA, operand 2 should be the same as operand 1. > Aka operand 2 should have RVVMF4QI mode as above. Thus, add > quad truncation for operand 2 before emit vwsll. > > The below test suites are passed for this patch. > * The rv64gcv fully regression test. > > PR target/116280 > > gcc/ChangeLog: > > * config/riscv/autovec-opt.md: Add quad truncation to > align the mode requirement for vwsll. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/rvv/base/pr116280-1.c: New test. > * gcc.target/riscv/rvv/base/pr116280-2.c: New test. Thanks. I've pushed this to the trunk. jeff
> Thanks. I've pushed this to the trunk. Thanks a lot, Jeff. Pan -----Original Message----- From: Jeff Law <jeffreyalaw@gmail.com> Sent: Saturday, August 17, 2024 11:27 PM To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; rdapp.gcc@gmail.com Subject: Re: [PATCH v1] RISC-V: Bugfix incorrect operand for vwsll auto-vect On 8/10/24 6:36 AM, pan2.li@intel.com wrote: > This patch would like to fix one ICE when rv64gcv_zvbb for vwsll. > Consider below example. > > void vwsll_vv_test (short *restrict dst, char *restrict a, > int *restrict b, int n) > { > for (int i = 0; i < n; i++) > dst[i] = a[i] << b[i]; > } > > It will hit the vwsll pattern with following operands. > operand 0 -> (reg:RVVMF2HI 146 [ vect__7.13 ]) > operand 1 -> (reg:RVVMF4QI 165 [ vect_cst__33 ]) > operand 2 -> (reg:RVVM1SI 171 [ vect_cst__36 ]) > > According to the ISA, operand 2 should be the same as operand 1. > Aka operand 2 should have RVVMF4QI mode as above. Thus, add > quad truncation for operand 2 before emit vwsll. > > The below test suites are passed for this patch. > * The rv64gcv fully regression test. > > PR target/116280 > > gcc/ChangeLog: > > * config/riscv/autovec-opt.md: Add quad truncation to > align the mode requirement for vwsll. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/rvv/base/pr116280-1.c: New test. > * gcc.target/riscv/rvv/base/pr116280-2.c: New test. Thanks. I've pushed this to the trunk. jeff
diff --git a/gcc/config/riscv/autovec-opt.md b/gcc/config/riscv/autovec-opt.md index d7a3cfd4602..4b33a145c17 100644 --- a/gcc/config/riscv/autovec-opt.md +++ b/gcc/config/riscv/autovec-opt.md @@ -1546,6 +1546,10 @@ (define_insn_and_split "*vwsll_zext1_trunc_<mode>" "&& 1" [(const_int 0)] { + rtx truncated = gen_reg_rtx (<V_QUAD_TRUNC>mode); + emit_insn (gen_trunc<mode><v_quad_trunc>2 (truncated, operands[2])); + operands[2] = truncated; + insn_code icode = code_for_pred_vwsll (<V_DOUBLE_TRUNC>mode); riscv_vector::emit_vlmax_insn (icode, riscv_vector::BINARY_OP, operands); DONE; diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr116280-1.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr116280-1.c new file mode 100644 index 00000000000..8b8547e2c34 --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr116280-1.c @@ -0,0 +1,14 @@ +/* Test there is no ICE when compile. */ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gcv_zvbb -mabi=lp64d -O3" } */ + +short a; +char b; + +void +test (int e[][1][1], char f[][1][1][1][1]) { + for (int g; b;) + for (;;) + for (int h; h < 4073709551572ULL; h += 18446744073709551612U) + a = f[2][2][1][4073709551612][1] << e[1][1][g]; +} diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr116280-2.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr116280-2.c new file mode 100644 index 00000000000..02f2de66eff --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr116280-2.c @@ -0,0 +1,10 @@ +/* Test there is no ICE when compile. */ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gcv_zvbb -mabi=lp64d -O3" } */ + +void +test (short *restrict dst, char *restrict a, int *restrict b, int n) +{ + for (int i = 0; i < n; i++) + dst[i] = a[i] << b[i]; +}