RISC-V: Optimal RVV epilogue logic.
Checks
Commit Message
Skip add insn generate if the adjust size equal to zero.
gcc/ChangeLog:
* config/riscv/riscv.cc (riscv_expand_epilogue):
New if control segement.
---
gcc/config/riscv/riscv.cc | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
Comments
Could you provide some testcase?
On Tue, Nov 15, 2022 at 12:29 AM jiawei <jiawei@iscas.ac.cn> wrote:
>
> Skip add insn generate if the adjust size equal to zero.
>
> gcc/ChangeLog:
>
> * config/riscv/riscv.cc (riscv_expand_epilogue):
> New if control segement.
>
> ---
> gcc/config/riscv/riscv.cc | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 02a01ca0b7c..af138db7545 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -5186,24 +5186,26 @@ riscv_expand_epilogue (int style)
> }
>
> /* Get an rtx for STEP1 that we can add to BASE. */
> - rtx adjust = GEN_INT (step1.to_constant ());
> - if (!SMALL_OPERAND (step1.to_constant ()))
> + if (step1.to_constant () != 0){
> + rtx adjust = GEN_INT (step1.to_constant ());
> + if (!SMALL_OPERAND (step1.to_constant ()))
> {
> riscv_emit_move (RISCV_PROLOGUE_TEMP (Pmode), adjust);
> adjust = RISCV_PROLOGUE_TEMP (Pmode);
> }
>
> - insn = emit_insn (
> + insn = emit_insn (
> gen_add3_insn (stack_pointer_rtx, stack_pointer_rtx, adjust));
>
> - rtx dwarf = NULL_RTX;
> - rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
> + rtx dwarf = NULL_RTX;
> + rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
> GEN_INT (step2));
>
> - dwarf = alloc_reg_note (REG_CFA_DEF_CFA, cfa_adjust_rtx, dwarf);
> - RTX_FRAME_RELATED_P (insn) = 1;
> + dwarf = alloc_reg_note (REG_CFA_DEF_CFA, cfa_adjust_rtx, dwarf);
> + RTX_FRAME_RELATED_P (insn) = 1;
>
> - REG_NOTES (insn) = dwarf;
> + REG_NOTES (insn) = dwarf;
> + }
> }
> else if (frame_pointer_needed)
> {
> --
> 2.25.1
>
> -----原始邮件-----
> 发件人: "Kito Cheng" <kito.cheng@gmail.com>
> 发送时间: 2022-11-15 09:48:26 (星期二)
> 收件人: jiawei <jiawei@iscas.ac.cn>
> 抄送: gcc-patches@gcc.gnu.org, kito.cheng@sifive.com, palmer@rivosinc.com, juzhe.zhong@rivai.ai, christoph.muellner@vrull.eu, philipp.tomsich@vrull.eu, wuwei2016@iscas.ac.cn
> 主题: Re: [PATCH] RISC-V: Optimal RVV epilogue logic.
>
> Could you provide some testcase?
Sorry for not giving a clear description，
You can use amost all testcases in gcc.target/riscv/rvv/base/spill-*.c
compile with -march=rv64gcv and check the assemble file spill-*.s,
before this patch, it will generate assemble code contain additional
`addi sp,sp,0`:
```
csrr t0,vlenb
slli t1,t0,1
add sp,sp,t1
addi sp,sp,0
ld s0,24(sp)
addi sp,sp,32
jr ra
```
after this patch it will removed:
```
csrr t0,vlenb
slli t1,t0,1
add sp,sp,t1
ld s0,24(sp)
addi sp,sp,32
jr ra
```
>
> On Tue, Nov 15, 2022 at 12:29 AM jiawei <jiawei@iscas.ac.cn> wrote:
> >
> > Skip add insn generate if the adjust size equal to zero.
> >
> > gcc/ChangeLog:
> >
> > * config/riscv/riscv.cc (riscv_expand_epilogue):
> > New if control segement.
> >
> > ---
> > gcc/config/riscv/riscv.cc | 18 ++++++++++--------
> > 1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > index 02a01ca0b7c..af138db7545 100644
> > --- a/gcc/config/riscv/riscv.cc
> > +++ b/gcc/config/riscv/riscv.cc
> > @@ -5186,24 +5186,26 @@ riscv_expand_epilogue (int style)
> > }
> >
> > /* Get an rtx for STEP1 that we can add to BASE. */
> > - rtx adjust = GEN_INT (step1.to_constant ());
> > - if (!SMALL_OPERAND (step1.to_constant ()))
> > + if (step1.to_constant () != 0){
> > + rtx adjust = GEN_INT (step1.to_constant ());
> > + if (!SMALL_OPERAND (step1.to_constant ()))
> > {
> > riscv_emit_move (RISCV_PROLOGUE_TEMP (Pmode), adjust);
> > adjust = RISCV_PROLOGUE_TEMP (Pmode);
> > }
> >
> > - insn = emit_insn (
> > + insn = emit_insn (
> > gen_add3_insn (stack_pointer_rtx, stack_pointer_rtx, adjust));
> >
> > - rtx dwarf = NULL_RTX;
> > - rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
> > + rtx dwarf = NULL_RTX;
> > + rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
> > GEN_INT (step2));
> >
> > - dwarf = alloc_reg_note (REG_CFA_DEF_CFA, cfa_adjust_rtx, dwarf);
> > - RTX_FRAME_RELATED_P (insn) = 1;
> > + dwarf = alloc_reg_note (REG_CFA_DEF_CFA, cfa_adjust_rtx, dwarf);
> > + RTX_FRAME_RELATED_P (insn) = 1;
> >
> > - REG_NOTES (insn) = dwarf;
> > + REG_NOTES (insn) = dwarf;
> > + }
> > }
> > else if (frame_pointer_needed)
> > {
> > --
> > 2.25.1
> >
</jiawei@iscas.ac.cn></jiawei@iscas.ac.cn></kito.cheng@gmail.com>
I think you'd better change assembler checking of "spill-*.c" cases.
Check they don't have "addi sp,sp,0" redundant instruction.
Let's see whether Kito aggree with that.
juzhe.zhong@rivai.ai
From: jiawei
Date: 2022-11-15 10:37
To: Kito Cheng
CC: gcc-patches; kito.cheng; palmer; juzhe.zhong; christoph.muellner; philipp.tomsich; wuwei2016
Subject: Re: Re: [PATCH] RISC-V: Optimal RVV epilogue logic.
> -----原始邮件-----
> 发件人: "Kito Cheng" <kito.cheng@gmail.com>
> 发送时间: 2022-11-15 09:48:26 (星期二)
> 收件人: jiawei <jiawei@iscas.ac.cn>
> 抄送: gcc-patches@gcc.gnu.org, kito.cheng@sifive.com, palmer@rivosinc.com, juzhe.zhong@rivai.ai, christoph.muellner@vrull.eu, philipp.tomsich@vrull.eu, wuwei2016@iscas.ac.cn
> 主题: Re: [PATCH] RISC-V: Optimal RVV epilogue logic.
>
> Could you provide some testcase?
Sorry for not giving a clear description，
You can use amost all testcases in gcc.target/riscv/rvv/base/spill-*.c
compile with -march=rv64gcv and check the assemble file spill-*.s,
before this patch, it will generate assemble code contain additional
`addi sp,sp,0`:
```
csrr t0,vlenb
slli t1,t0,1
add sp,sp,t1
addi sp,sp,0
ld s0,24(sp)
addi sp,sp,32
jr ra
```
after this patch it will removed:
```
csrr t0,vlenb
slli t1,t0,1
add sp,sp,t1
ld s0,24(sp)
addi sp,sp,32
jr ra
```
>
> On Tue, Nov 15, 2022 at 12:29 AM jiawei <jiawei@iscas.ac.cn> wrote:
> >
> > Skip add insn generate if the adjust size equal to zero.
> >
> > gcc/ChangeLog:
> >
> > * config/riscv/riscv.cc (riscv_expand_epilogue):
> > New if control segement.
> >
> > ---
> > gcc/config/riscv/riscv.cc | 18 ++++++++++--------
> > 1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > index 02a01ca0b7c..af138db7545 100644
> > --- a/gcc/config/riscv/riscv.cc
> > +++ b/gcc/config/riscv/riscv.cc
> > @@ -5186,24 +5186,26 @@ riscv_expand_epilogue (int style)
> > }
> >
> > /* Get an rtx for STEP1 that we can add to BASE. */
> > - rtx adjust = GEN_INT (step1.to_constant ());
> > - if (!SMALL_OPERAND (step1.to_constant ()))
> > + if (step1.to_constant () != 0){
> > + rtx adjust = GEN_INT (step1.to_constant ());
> > + if (!SMALL_OPERAND (step1.to_constant ()))
> > {
> > riscv_emit_move (RISCV_PROLOGUE_TEMP (Pmode), adjust);
> > adjust = RISCV_PROLOGUE_TEMP (Pmode);
> > }
> >
> > - insn = emit_insn (
> > + insn = emit_insn (
> > gen_add3_insn (stack_pointer_rtx, stack_pointer_rtx, adjust));
> >
> > - rtx dwarf = NULL_RTX;
> > - rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
> > + rtx dwarf = NULL_RTX;
> > + rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
> > GEN_INT (step2));
> >
> > - dwarf = alloc_reg_note (REG_CFA_DEF_CFA, cfa_adjust_rtx, dwarf);
> > - RTX_FRAME_RELATED_P (insn) = 1;
> > + dwarf = alloc_reg_note (REG_CFA_DEF_CFA, cfa_adjust_rtx, dwarf);
> > + RTX_FRAME_RELATED_P (insn) = 1;
> >
> > - REG_NOTES (insn) = dwarf;
> > + REG_NOTES (insn) = dwarf;
> > + }
> > }
> > else if (frame_pointer_needed)
> > {
> > --
> > 2.25.1
> >
</jiawei@iscas.ac.cn></jiawei@iscas.ac.cn></kito.cheng@gmail.com>
I would suggest add a sperated case and scan-assembly-not to demonstrate
this patch.
juzhe.zhong@rivai.ai <juzhe.zhong@rivai.ai> 於 2022年11月15日 週二 10:47 寫道：
> I think you'd better change assembler checking of "spill-*.c" cases.
> Check they don't have "addi sp,sp,0" redundant instruction.
> Let's see whether Kito aggree with that.
> ------------------------------
> juzhe.zhong@rivai.ai
>
>
> *From:* jiawei <jiawei@iscas.ac.cn>
> *Date:* 2022-11-15 10:37
> *To:* Kito Cheng <kito.cheng@gmail.com>
> *CC:* gcc-patches <gcc-patches@gcc.gnu.org>; kito.cheng
> <kito.cheng@sifive.com>; palmer <palmer@rivosinc.com>; juzhe.zhong
> <juzhe.zhong@rivai.ai>; christoph.muellner <christoph.muellner@vrull.eu>;
> philipp.tomsich <philipp.tomsich@vrull.eu>; wuwei2016
> <wuwei2016@iscas.ac.cn>
> *Subject:* Re: Re: [PATCH] RISC-V: Optimal RVV epilogue logic.
> > -----原始邮件-----
> > 发件人: "Kito Cheng" <kito.cheng@gmail.com>
> > 发送时间: 2022-11-15 09:48:26 (星期二)
> > 收件人: jiawei <jiawei@iscas.ac.cn>
> > 抄送: gcc-patches@gcc.gnu.org, kito.cheng@sifive.com,
> palmer@rivosinc.com, juzhe.zhong@rivai.ai, christoph.muellner@vrull.eu,
> philipp.tomsich@vrull.eu, wuwei2016@iscas.ac.cn
> > 主题: Re: [PATCH] RISC-V: Optimal RVV epilogue logic.
> >
> > Could you provide some testcase?
>
> Sorry for not giving a clear description，
>
> You can use amost all testcases in gcc.target/riscv/rvv/base/spill-*.c
>
> compile with -march=rv64gcv and check the assemble file spill-*.s,
>
> before this patch, it will generate assemble code contain additional
>
> `addi sp,sp,0`:
>
> ```
> csrr t0,vlenb
> slli t1,t0,1
> add sp,sp,t1
> addi sp,sp,0
> ld s0,24(sp)
> addi sp,sp,32
> jr ra
> ```
>
> after this patch it will removed:
>
> ```
> csrr t0,vlenb
> slli t1,t0,1
> add sp,sp,t1
> ld s0,24(sp)
> addi sp,sp,32
> jr ra
> ```
>
> >
> > On Tue, Nov 15, 2022 at 12:29 AM jiawei <jiawei@iscas.ac.cn> wrote:
> > >
> > > Skip add insn generate if the adjust size equal to zero.
> > >
> > > gcc/ChangeLog:
> > >
> > > * config/riscv/riscv.cc (riscv_expand_epilogue):
> > > New if control segement.
> > >
> > > ---
> > > gcc/config/riscv/riscv.cc | 18 ++++++++++--------
> > > 1 file changed, 10 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/gcc/config/riscv/riscv.cc
> b/gcc/config/riscv/riscv.cc
> > > index 02a01ca0b7c..af138db7545 100644
> > > --- a/gcc/config/riscv/riscv.cc
> > > +++ b/gcc/config/riscv/riscv.cc
> > > @@ -5186,24 +5186,26 @@ riscv_expand_epilogue (int style)
> > > }
> > >
> > > /* Get an rtx for STEP1 that we can add to BASE. */
> > > - rtx adjust = GEN_INT (step1.to_constant ());
> > > - if (!SMALL_OPERAND (step1.to_constant ()))
> > > + if (step1.to_constant () != 0){
> > > + rtx adjust = GEN_INT (step1.to_constant ());
> > > + if (!SMALL_OPERAND (step1.to_constant ()))
> > > {
> > > riscv_emit_move (RISCV_PROLOGUE_TEMP (Pmode), adjust);
> > > adjust = RISCV_PROLOGUE_TEMP (Pmode);
> > > }
> > >
> > > - insn = emit_insn (
> > > + insn = emit_insn (
> > > gen_add3_insn (stack_pointer_rtx,
> stack_pointer_rtx, adjust));
> > >
> > > - rtx dwarf = NULL_RTX;
> > > - rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode,
> stack_pointer_rtx,
> > > + rtx dwarf = NULL_RTX;
> > > + rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode,
> stack_pointer_rtx,
> > > GEN_INT (step2));
> > >
> > > - dwarf = alloc_reg_note (REG_CFA_DEF_CFA, cfa_adjust_rtx,
> dwarf);
> > > - RTX_FRAME_RELATED_P (insn) = 1;
> > > + dwarf = alloc_reg_note (REG_CFA_DEF_CFA,
> cfa_adjust_rtx, dwarf);
> > > + RTX_FRAME_RELATED_P (insn) = 1;
> > >
> > > - REG_NOTES (insn) = dwarf;
> > > + REG_NOTES (insn) = dwarf;
> > > + }
> > > }
> > > else if (frame_pointer_needed)
> > > {
> > > --
> > > 2.25.1
> > >
> </jiawei@iscas.ac.cn></jiawei@iscas.ac.cn></kito.cheng@gmail.com>
>
>
On 11/14/22 09:29, jiawei wrote:
> Skip add insn generate if the adjust size equal to zero.
>
> gcc/ChangeLog:
>
> * config/riscv/riscv.cc (riscv_expand_epilogue):
> New if control segement.
>
> ---
> gcc/config/riscv/riscv.cc | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 02a01ca0b7c..af138db7545 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -5186,24 +5186,26 @@ riscv_expand_epilogue (int style)
> }
>
> /* Get an rtx for STEP1 that we can add to BASE. */
> - rtx adjust = GEN_INT (step1.to_constant ());
> - if (!SMALL_OPERAND (step1.to_constant ()))
> + if (step1.to_constant () != 0){
This doesn't follow GCC formatting rules. The open-curley should go on
a new line, intended two spaces further in. This will (of course) cause
other code to need to be reindented as well.
Jeff
On 11/14/22 20:13, Kito Cheng wrote:
> I would suggest add a sperated case and scan-assembly-not to demonstrate
> this patch.
Agreed. One way to do this would be to have new tests which have the
proper dg-directives for testing this issue and #include the original test.
So, something like this:
/* { dg-do compile } */
/* { dg-options "-march=rv32gcv -mabi=ilp32 -mpreferred-stack-boundary=3
-O3 -fno-schedule-insns -fno-schedule-insns2" } */
#include "spill-1.c"
/* Make sure we do not have a useless SP adjustment. */
/* { dg-final { scan-assembler-not "addi sp, sp, 0" } } */
The key thing to know is that the dg directives are parsed by the
framework before preprocessing. So the dg-directives in spill-1.c would
not affect this new test. That requires us to provide our own, both for
how to run the test and what to look for.
Jeff
On Mon, 14 Nov 2022 at 17:29, jiawei <jiawei@iscas.ac.cn> wrote:
>
> Skip add insn generate if the adjust size equal to zero.
>
> gcc/ChangeLog:
>
> * config/riscv/riscv.cc (riscv_expand_epilogue):
> New if control segement.
>
> ---
> gcc/config/riscv/riscv.cc | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 02a01ca0b7c..af138db7545 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -5186,24 +5186,26 @@ riscv_expand_epilogue (int style)
> }
>
> /* Get an rtx for STEP1 that we can add to BASE. */
> - rtx adjust = GEN_INT (step1.to_constant ());
> - if (!SMALL_OPERAND (step1.to_constant ()))
> + if (step1.to_constant () != 0){
> + rtx adjust = GEN_INT (step1.to_constant ());
> + if (!SMALL_OPERAND (step1.to_constant ()))
Please take a look at the recent improvements for the add<mode>3
expander (recently submitted as
https://patchwork.ozlabs.org/project/gcc/patch/20221109230718.3240479-1-philipp.tomsich@vrull.eu/).
Maybe you also want to use the test for the addi_operand(...) instead
of SMALL_OPERAND?
> {
> riscv_emit_move (RISCV_PROLOGUE_TEMP (Pmode), adjust);
> adjust = RISCV_PROLOGUE_TEMP (Pmode);
> }
>
> - insn = emit_insn (
> + insn = emit_insn (
> gen_add3_insn (stack_pointer_rtx, stack_pointer_rtx, adjust));
>
> - rtx dwarf = NULL_RTX;
> - rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
> + rtx dwarf = NULL_RTX;
> + rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
> GEN_INT (step2));
>
> - dwarf = alloc_reg_note (REG_CFA_DEF_CFA, cfa_adjust_rtx, dwarf);
> - RTX_FRAME_RELATED_P (insn) = 1;
> + dwarf = alloc_reg_note (REG_CFA_DEF_CFA, cfa_adjust_rtx, dwarf);
> + RTX_FRAME_RELATED_P (insn) = 1;
>
> - REG_NOTES (insn) = dwarf;
> + REG_NOTES (insn) = dwarf;
> + }
> }
> else if (frame_pointer_needed)
> {
> --
> 2.25.1
>
@@ -5186,24 +5186,26 @@ riscv_expand_epilogue (int style)
}
/* Get an rtx for STEP1 that we can add to BASE. */
- rtx adjust = GEN_INT (step1.to_constant ());
- if (!SMALL_OPERAND (step1.to_constant ()))
+ if (step1.to_constant () != 0){
+ rtx adjust = GEN_INT (step1.to_constant ());
+ if (!SMALL_OPERAND (step1.to_constant ()))
{
riscv_emit_move (RISCV_PROLOGUE_TEMP (Pmode), adjust);
adjust = RISCV_PROLOGUE_TEMP (Pmode);
}
- insn = emit_insn (
+ insn = emit_insn (
gen_add3_insn (stack_pointer_rtx, stack_pointer_rtx, adjust));
- rtx dwarf = NULL_RTX;
- rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
+ rtx dwarf = NULL_RTX;
+ rtx cfa_adjust_rtx = gen_rtx_PLUS (Pmode, stack_pointer_rtx,
GEN_INT (step2));
- dwarf = alloc_reg_note (REG_CFA_DEF_CFA, cfa_adjust_rtx, dwarf);
- RTX_FRAME_RELATED_P (insn) = 1;
+ dwarf = alloc_reg_note (REG_CFA_DEF_CFA, cfa_adjust_rtx, dwarf);
+ RTX_FRAME_RELATED_P (insn) = 1;
- REG_NOTES (insn) = dwarf;
+ REG_NOTES (insn) = dwarf;
+ }
}
else if (frame_pointer_needed)
{