diff mbox series

[v1] RISC-V: Bugfix incorrect operand for vwsll auto-vect

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

Commit Message

Li, Pan2 Aug. 10, 2024, 12:36 p.m. UTC 
From: Pan Li <pan2.li@intel.com>

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.

Signed-off-by: Pan Li <pan2.li@intel.com>
---
 gcc/config/riscv/autovec-opt.md                    |  4 ++++
 .../gcc.target/riscv/rvv/base/pr116280-1.c         | 14 ++++++++++++++
 .../gcc.target/riscv/rvv/base/pr116280-2.c         | 10 ++++++++++
 3 files changed, 28 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr116280-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr116280-2.c

Comments

Robin Dapp Aug. 10, 2024, 2:31 p.m. UTC | #1 
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?
Li, Pan2 Aug. 11, 2024, 12:10 a.m. UTC | #2 
> 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?
Jeff Law Aug. 17, 2024, 3:24 p.m. UTC | #3 
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
Jeff Law Aug. 17, 2024, 3:26 p.m. UTC | #4 
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
Li, Pan2 Aug. 18, 2024, 1:04 a.m. UTC | #5 
> 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 mbox series

Patch

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];
+}