| Message ID | 51AE84BB9E509EBC+202409111936098703655@rivai.ai |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] RISC-V: Fix vl_used_by_non_rvv_insn logic of vsetvl pass | expand |
Committed, thanks Juzhe and garthlei.
Pan
From: 钟居哲 <juzhe.zhong@rivai.ai>
Sent: Wednesday, September 11, 2024 7:36 PM
To: gcc-patches <gcc-patches@gcc.gnu.org>
Cc: Li, Pan2 <pan2.li@intel.com>; Robin Dapp <rdapp.gcc@gmail.com>; jeffreyalaw <jeffreyalaw@gmail.com>; kito.cheng <kito.cheng@gmail.com>
Subject: [PATCH 1/2] RISC-V: Fix vl_used_by_non_rvv_insn logic of vsetvl pass
Hi, garthlei.
Thanks for fixing it.
I see, you are trying to fix this bug:
lui a5,%hi(.LANCHOR0)
addi a5,a5,%lo(.LANCHOR0)
vsetivli zero,2,e8,mf8,ta,ma ---> It should be a4, 2 instead of zero, 2
vle64.v v1,0(a5)
--- missing vsetvli a4, a4 here
slli a4,a4,1
vsetvli zero,a4,e32,m1,ta,ma
li a2,-1
addi a5,a5,16
vslide1down.vx v1,v1,a2
vslide1down.vx v1,v1,zero
vsetivli zero,2,e64,m1,ta,ma
vse64.v v1,0(a5)
ret
When I revisit the codes here:
m_vl = ::get_vl
...
update_avl -> "m_vl" variable is modified
...
using wrong m_vl in the following.
A dedicated temporary variable dest_vl looks reasonable here.
LGTM.
The RISC-V folks will commit this patch for you.
Thanks.
diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc index 017efa8bc17..ce831685439 100644 --- a/gcc/config/riscv/riscv-vsetvl.cc +++ b/gcc/config/riscv/riscv-vsetvl.cc @@ -1002,6 +1002,9 @@ public: void parse_insn (insn_info *insn) { + /* The VL dest of the insn */ + rtx dest_vl = NULL_RTX; + m_insn = insn; m_bb = insn->bb (); /* Return if it is debug insn for the consistency with optimize == 0. */ @@ -1035,7 +1038,10 @@ public: if (m_avl) { if (vsetvl_insn_p (insn->rtl ()) || has_vlmax_avl ()) - m_vl = ::get_vl (insn->rtl ()); + { + m_vl = ::get_vl (insn->rtl ()); + dest_vl = m_vl; + } if (has_nonvlmax_reg_avl ()) m_avl_def = find_access (insn->uses (), REGNO (m_avl))->def (); @@ -1132,22 +1138,22 @@ public: } /* Determine if dest operand(vl) has been used by non-RVV instructions. */ - if (has_vl ()) + if (dest_vl) { const hash_set<use_info *> vl_uses - = get_all_real_uses (get_insn (), REGNO (get_vl ())); + = get_all_real_uses (get_insn (), REGNO (dest_vl)); for (use_info *use : vl_uses) { gcc_assert (use->insn ()->is_real ()); rtx_insn *rinsn = use->insn ()->rtl (); if (!has_vl_op (rinsn) - || count_regno_occurrences (rinsn, REGNO (get_vl ())) != 1) + || count_regno_occurrences (rinsn, REGNO (dest_vl)) != 1) { m_vl_used_by_non_rvv_insn = true; break; } rtx avl = ::get_avl (rinsn); - if (!avl || !REG_P (avl) || REGNO (get_vl ()) != REGNO (avl)) + if (!avl || !REG_P (avl) || REGNO (dest_vl) != REGNO (avl)) { m_vl_used_by_non_rvv_insn = true; break; diff --git a/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vsetvl_bug-3.c b/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vsetvl_bug-3.c new file mode 100644 index 00000000000..c155f5613d2 --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vsetvl_bug-3.c @@ -0,0 +1,17 @@ +/* { dg-do compile } */ +/* { dg-options "-march=rv32gcv -mabi=ilp32d -O2 -fdump-rtl-vsetvl-details" } */ + +#include <riscv_vector.h> + +uint64_t a[2], b[2]; + +void +foo () +{ + size_t vl = __riscv_vsetvl_e64m1 (2); + vuint64m1_t vx = __riscv_vle64_v_u64m1 (a, vl); + vx = __riscv_vslide1down_vx_u64m1 (vx, 0xffffffffull, vl); + __riscv_vse64_v_u64m1 (b, vx, vl); +} + +/* { dg-final { scan-rtl-dump-not "Eliminate insn" "vsetvl" } } */
Hi, garthlei. Thanks for fixing it. I see, you are trying to fix this bug: lui a5,%hi(.LANCHOR0) addi a5,a5,%lo(.LANCHOR0) vsetivli zero,2,e8,mf8,ta,ma ---> It should be a4, 2 instead of zero, 2 vle64.v v1,0(a5) --- missing vsetvli a4, a4 here slli a4,a4,1 vsetvli zero,a4,e32,m1,ta,ma li a2,-1 addi a5,a5,16 vslide1down.vx v1,v1,a2 vslide1down.vx v1,v1,zero vsetivli zero,2,e64,m1,ta,ma vse64.v v1,0(a5) ret When I revisit the codes here: m_vl = ::get_vl ... update_avl -> "m_vl" variable is modified ... using wrong m_vl in the following. A dedicated temporary variable dest_vl looks reasonable here. LGTM. The RISC-V folks will commit this patch for you. Thanks. juzhe.zhong@rivai.ai From: Li, Pan2 Date: 2024-09-11 19:29 To: juzhe.zhong@rivai.ai Subject: FW: [PATCH 1/2] RISC-V: Fix vl_used_by_non_rvv_insn logic of vsetvl pass FYI. -----Original Message----- From: garthlei <garthlei@linux.alibaba.com> Sent: Wednesday, September 11, 2024 5:10 PM To: gcc-patches <gcc-patches@gcc.gnu.org> Subject: [PATCH 1/2] RISC-V: Fix vl_used_by_non_rvv_insn logic of vsetvl pass This patch fixes a bug in the current vsetvl pass. The current pass uses `m_vl` to determine whether the dest operand has been used by non-RVV instructions. However, `m_vl` may have been modified as a result of an `update_avl` call, and thus would be no longer the dest operand of the original instruction. This can lead to incorrect vsetvl eliminations, as is shown in the testcase. In this patch, we create a `dest_vl` variable for this scenerio. gcc/ChangeLog: * config/riscv/riscv-vsetvl.cc: Use `dest_vl` for dest VL operand gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/vsetvl/vsetvl_bug-3.c: New test. --- gcc/config/riscv/riscv-vsetvl.cc | 16 +++++++++++----- .../gcc.target/riscv/rvv/vsetvl/vsetvl_bug-3.c | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vsetvl_bug-3.c