| Message ID | 20230524015727.1157568-1-juzhe.zhong@rivai.ai |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Fix incorrect code of touching inaccessible memory address | expand |
I am a little hesitant about that, since I feel the vl and normal op should be put in separately, otherwise the means of m_op_num is kind of unclear, we have comments there but I think it's not ideal since it is really context sensitive and hard to determine. And I suspect gcc_assert (ops[m_op_num]); is not too useful since it might just be out of range access if we forgot to pass the vl operands. I am thinking we might need to introduce something like llvm::ArrayRef to have a better sanity check, e.g. check the length of ops. One possible solution is just using std::vector can achieve the same purpose too, but come with more cost. On Wed, May 24, 2023 at 9:57 AM <juzhe.zhong@rivai.ai> wrote: > > From: Juzhe-Zhong <juzhe.zhong@rivai.ai> > > For VLMAX situation, rtx len = ops[m_op_num] is incorrect since > the last element the ops array should be ops[m_op_num - 1]; > > I notice this issue when I am debugging code. > This is a code bug even though the following codes will hide this issue. > We still should need this minor fix. > > Built && Regression PASSed. > > Ok for trunk? > > gcc/ChangeLog: > > * config/riscv/riscv-v.cc: Fix bug of touching inaccessible memory. > > --- > gcc/config/riscv/riscv-v.cc | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc > index fa61a850a22..a0992773644 100644 > --- a/gcc/config/riscv/riscv-v.cc > +++ b/gcc/config/riscv/riscv-v.cc > @@ -169,7 +169,11 @@ public: > > if (m_needs_avl_p) > { > - rtx len = ops[m_op_num]; > + /* The variable "m_op_num" means the real operation operands except VL > + operand. For VLMAX patterns (no VL operand), the last operand is > + ops[m_op_num -1]. Wheras for non-VLMAX patterns, the last operand is > + VL operand which is ops[m_op_num]. */ > + rtx len = NULL_RTX; > if (m_vlmax_p) > { > if (const_vlmax_p (m_dest_mode)) > @@ -185,6 +189,20 @@ public: > len = gen_reg_rtx (Pmode); > emit_vlmax_vsetvl (m_dest_mode, len); > } > + else > + { > + /* According to LRA mov pattern in vector.md. The VL operand is > + always the last operand. */ > + gcc_assert (ops[m_op_num]); > + len = ops[m_op_num]; > + } > + } > + else > + { > + /* For non-VLMAX patterns. The VL operand is always the last > + * operand. */ > + gcc_assert (ops[m_op_num]); > + len = ops[m_op_num]; > } > add_input_operand (len, Pmode); > } > -- > 2.36.3 >
Thanks. I fix it by separating VL and normal operand. V2 patch: https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619356.html Does it look more reasonable to you? Just finished the building test && regression. Thanks. juzhe.zhong@rivai.ai From: Kito Cheng Date: 2023-05-24 10:10 To: juzhe.zhong CC: gcc-patches; kito.cheng; palmer; palmer; jeffreyalaw; rdapp.gcc Subject: Re: [PATCH] RISC-V: Fix incorrect code of touching inaccessible memory address I am a little hesitant about that, since I feel the vl and normal op should be put in separately, otherwise the means of m_op_num is kind of unclear, we have comments there but I think it's not ideal since it is really context sensitive and hard to determine. And I suspect gcc_assert (ops[m_op_num]); is not too useful since it might just be out of range access if we forgot to pass the vl operands. I am thinking we might need to introduce something like llvm::ArrayRef to have a better sanity check, e.g. check the length of ops. One possible solution is just using std::vector can achieve the same purpose too, but come with more cost. On Wed, May 24, 2023 at 9:57 AM <juzhe.zhong@rivai.ai> wrote: > > From: Juzhe-Zhong <juzhe.zhong@rivai.ai> > > For VLMAX situation, rtx len = ops[m_op_num] is incorrect since > the last element the ops array should be ops[m_op_num - 1]; > > I notice this issue when I am debugging code. > This is a code bug even though the following codes will hide this issue. > We still should need this minor fix. > > Built && Regression PASSed. > > Ok for trunk? > > gcc/ChangeLog: > > * config/riscv/riscv-v.cc: Fix bug of touching inaccessible memory. > > --- > gcc/config/riscv/riscv-v.cc | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc > index fa61a850a22..a0992773644 100644 > --- a/gcc/config/riscv/riscv-v.cc > +++ b/gcc/config/riscv/riscv-v.cc > @@ -169,7 +169,11 @@ public: > > if (m_needs_avl_p) > { > - rtx len = ops[m_op_num]; > + /* The variable "m_op_num" means the real operation operands except VL > + operand. For VLMAX patterns (no VL operand), the last operand is > + ops[m_op_num -1]. Wheras for non-VLMAX patterns, the last operand is > + VL operand which is ops[m_op_num]. */ > + rtx len = NULL_RTX; > if (m_vlmax_p) > { > if (const_vlmax_p (m_dest_mode)) > @@ -185,6 +189,20 @@ public: > len = gen_reg_rtx (Pmode); > emit_vlmax_vsetvl (m_dest_mode, len); > } > + else > + { > + /* According to LRA mov pattern in vector.md. The VL operand is > + always the last operand. */ > + gcc_assert (ops[m_op_num]); > + len = ops[m_op_num]; > + } > + } > + else > + { > + /* For non-VLMAX patterns. The VL operand is always the last > + * operand. */ > + gcc_assert (ops[m_op_num]); > + len = ops[m_op_num]; > } > add_input_operand (len, Pmode); > } > -- > 2.36.3 >
diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc index fa61a850a22..a0992773644 100644 --- a/gcc/config/riscv/riscv-v.cc +++ b/gcc/config/riscv/riscv-v.cc @@ -169,7 +169,11 @@ public: if (m_needs_avl_p) { - rtx len = ops[m_op_num]; + /* The variable "m_op_num" means the real operation operands except VL + operand. For VLMAX patterns (no VL operand), the last operand is + ops[m_op_num -1]. Wheras for non-VLMAX patterns, the last operand is + VL operand which is ops[m_op_num]. */ + rtx len = NULL_RTX; if (m_vlmax_p) { if (const_vlmax_p (m_dest_mode)) @@ -185,6 +189,20 @@ public: len = gen_reg_rtx (Pmode); emit_vlmax_vsetvl (m_dest_mode, len); } + else + { + /* According to LRA mov pattern in vector.md. The VL operand is + always the last operand. */ + gcc_assert (ops[m_op_num]); + len = ops[m_op_num]; + } + } + else + { + /* For non-VLMAX patterns. The VL operand is always the last + * operand. */ + gcc_assert (ops[m_op_num]); + len = ops[m_op_num]; } add_input_operand (len, Pmode); }
From: Juzhe-Zhong <juzhe.zhong@rivai.ai> For VLMAX situation, rtx len = ops[m_op_num] is incorrect since the last element the ops array should be ops[m_op_num - 1]; I notice this issue when I am debugging code. This is a code bug even though the following codes will hide this issue. We still should need this minor fix. Built && Regression PASSed. Ok for trunk? gcc/ChangeLog: * config/riscv/riscv-v.cc: Fix bug of touching inaccessible memory. --- gcc/config/riscv/riscv-v.cc | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)