Message ID | 3cc5403de383d7c8cfd1769948c2bcf9d54b97f9.1693786829.git.research_trasio@irq.a4lg.com |
---|---|
State | New |
Headers | show |
Series | RISC-V: Fix Zicond ICE on large constants | expand |
Maybe move the check logic a bit forward? My thought is the logic is already specialized into a few catalogs, (imm, imm), (imm, reg), (reg, reg)... and the logic you put is already in (imm, reg), but it should really move into (reg, reg) case IMO? and move that forward we could prevent add too much logic to redirect the case. diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc index 2db9c81ac8b..c84509c393b 100644 --- a/gcc/config/riscv/riscv.cc +++ b/gcc/config/riscv/riscv.cc @@ -3892,6 +3892,12 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx cons, rtx alt) op1 = XEXP (op, 1); } + /* CONS might not fit into a signed 12 bit immediate suitable + for an addi instruction. If that's the case, force it into + a register. */ + if (CONST_INT_P (cons) && !SMALL_OPERAND (INTVAL (cons))) + cons = force_reg (mode, cons); + /* 0, reg or 0, imm */ if (cons == CONST0_RTX (mode) && (REG_P (alt) On Mon, Sep 4, 2023 at 8:21 AM Tsukasa OI via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > From: Tsukasa OI <research_trasio@irq.a4lg.com> > > Large constant cons and/or alt will trigger ICEs building GCC target > libraries (libgomp and libatomic) when the 'Zicond' extension is enabled. > > For instance, zicond-ice-2.c (new test case in this commit) will cause > an ICE when SOME_NUMBER is 0x1000 or larger. While opposite numbers > corresponding cons/alt (two temp2 variables) are checked, cons/alt > themselves are not checked and causing 2 ICEs building > GCC target libraries as of this writing: > > 1. gcc/libatomic/config/posix/lock.c > 2. gcc/libgomp/fortran.c > > Coercing a large value into a register will fix the issue. > > gcc/ChangeLog: > > * config/riscv/riscv.cc (riscv_expand_conditional_move): Force > large constant cons/alt into a register. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/zicond-ice-2.c: New test. This is based on > an ICE at libat_lock_n func on gcc/libatomic/config/posix/lock.c > but heavily minimized. > --- > gcc/config/riscv/riscv.cc | 16 ++++++++++------ > gcc/testsuite/gcc.target/riscv/zicond-ice-2.c | 11 +++++++++++ > 2 files changed, 21 insertions(+), 6 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-ice-2.c > > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc > index 8d8f7b4f16ed..cfaa4b6a7720 100644 > --- a/gcc/config/riscv/riscv.cc > +++ b/gcc/config/riscv/riscv.cc > @@ -3940,11 +3940,13 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx cons, rtx alt) > rtx temp1 = gen_reg_rtx (mode); > rtx temp2 = gen_int_mode (-1 * INTVAL (cons), mode); > > - /* TEMP2 might not fit into a signed 12 bit immediate suitable > - for an addi instruction. If that's the case, force it into > - a register. */ > + /* TEMP2 and/or CONS might not fit into a signed 12 bit immediate > + suitable for an addi instruction. If that's the case, force it > + into a register. */ > if (!SMALL_OPERAND (INTVAL (temp2))) > temp2 = force_reg (mode, temp2); > + if (!SMALL_OPERAND (INTVAL (cons))) > + cons = force_reg (mode, cons); > > riscv_emit_binary (PLUS, temp1, alt, temp2); > emit_insn (gen_rtx_SET (dest, > @@ -3986,11 +3988,13 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx cons, rtx alt) > rtx temp1 = gen_reg_rtx (mode); > rtx temp2 = gen_int_mode (-1 * INTVAL (alt), mode); > > - /* TEMP2 might not fit into a signed 12 bit immediate suitable > - for an addi instruction. If that's the case, force it into > - a register. */ > + /* TEMP2 and/or ALT might not fit into a signed 12 bit immediate > + suitable for an addi instruction. If that's the case, force it > + into a register. */ > if (!SMALL_OPERAND (INTVAL (temp2))) > temp2 = force_reg (mode, temp2); > + if (!SMALL_OPERAND (INTVAL (alt))) > + alt = force_reg (mode, alt); > > riscv_emit_binary (PLUS, temp1, cons, temp2); > emit_insn (gen_rtx_SET (dest, > diff --git a/gcc/testsuite/gcc.target/riscv/zicond-ice-2.c b/gcc/testsuite/gcc.target/riscv/zicond-ice-2.c > new file mode 100644 > index 000000000000..ffd8dcb5814e > --- /dev/null > +++ b/gcc/testsuite/gcc.target/riscv/zicond-ice-2.c > @@ -0,0 +1,11 @@ > +/* { dg-do compile } */ > +/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */ > +/* { dg-options "-march=rv32gc_zicond -mabi=ilp32d" { target { rv32 } } } */ > + > +#define SOME_NUMBER 0x1000 > + > +unsigned long > +d (unsigned long n) > +{ > + return n > SOME_NUMBER ? SOME_NUMBER : n; > +} > > base-commit: 78f636d979530c8a649262dbd44914bdfb6f7290 > -- > 2.42.0 >
On 9/4/23 00:45, Kito Cheng wrote: > Maybe move the check logic a bit forward? My thought is the logic is > already specialized into a few catalogs, (imm, imm), (imm, reg), (reg, > reg)... and the logic you put is already in (imm, reg), but it should > really move into (reg, reg) case IMO? and move that forward we could > prevent add too much logic to redirect the case. > > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc > index 2db9c81ac8b..c84509c393b 100644 > --- a/gcc/config/riscv/riscv.cc > +++ b/gcc/config/riscv/riscv.cc > @@ -3892,6 +3892,12 @@ riscv_expand_conditional_move (rtx dest, rtx > op, rtx cons, rtx alt) > op1 = XEXP (op, 1); > } > > + /* CONS might not fit into a signed 12 bit immediate suitable > + for an addi instruction. If that's the case, force it into > + a register. */ > + if (CONST_INT_P (cons) && !SMALL_OPERAND (INTVAL (cons))) > + cons = force_reg (mode, cons); > + > /* 0, reg or 0, imm */ > if (cons == CONST0_RTX (mode) > && (REG_P (alt) But for the imm, imm case if we force things into regs too early, then we'll lose if alt - cons and cons fit in a 12 bit immediate but alt does not. I think Tsukasa is on the right path here. I should have checked riscv_emit_binary -- I though it handled the out-of-range constant case, but looking at it now, it clearly does not. I think this implies we need a similar blob of code for the imm, imm case for cons. Jeff
On 2023/09/05 14:27, Jeff Law wrote: > > > On 9/4/23 00:45, Kito Cheng wrote: >> Maybe move the check logic a bit forward? My thought is the logic is >> already specialized into a few catalogs, (imm, imm), (imm, reg), (reg, >> reg)... and the logic you put is already in (imm, reg), but it should >> really move into (reg, reg) case IMO? and move that forward we could >> prevent add too much logic to redirect the case. >> >> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc >> index 2db9c81ac8b..c84509c393b 100644 >> --- a/gcc/config/riscv/riscv.cc >> +++ b/gcc/config/riscv/riscv.cc >> @@ -3892,6 +3892,12 @@ riscv_expand_conditional_move (rtx dest, rtx >> op, rtx cons, rtx alt) >> op1 = XEXP (op, 1); >> } >> >> + /* CONS might not fit into a signed 12 bit immediate suitable >> + for an addi instruction. If that's the case, force it into >> + a register. */ >> + if (CONST_INT_P (cons) && !SMALL_OPERAND (INTVAL (cons))) >> + cons = force_reg (mode, cons); >> + >> /* 0, reg or 0, imm */ >> if (cons == CONST0_RTX (mode) >> && (REG_P (alt) > But for the imm, imm case if we force things into regs too early, then > we'll lose if alt - cons and cons fit in a 12 bit immediate but alt does > not. > > I think Tsukasa is on the right path here. I should have checked > riscv_emit_binary -- I though it handled the out-of-range constant case, > but looking at it now, it clearly does not. > > I think this implies we need a similar blob of code for the imm, imm > case for cons. > > Jeff > Okay, adding a check to "imm, imm" case (although I haven't figured out how to reproduce this case) and will submit the v2. Tsukasa
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc index 8d8f7b4f16ed..cfaa4b6a7720 100644 --- a/gcc/config/riscv/riscv.cc +++ b/gcc/config/riscv/riscv.cc @@ -3940,11 +3940,13 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx cons, rtx alt) rtx temp1 = gen_reg_rtx (mode); rtx temp2 = gen_int_mode (-1 * INTVAL (cons), mode); - /* TEMP2 might not fit into a signed 12 bit immediate suitable - for an addi instruction. If that's the case, force it into - a register. */ + /* TEMP2 and/or CONS might not fit into a signed 12 bit immediate + suitable for an addi instruction. If that's the case, force it + into a register. */ if (!SMALL_OPERAND (INTVAL (temp2))) temp2 = force_reg (mode, temp2); + if (!SMALL_OPERAND (INTVAL (cons))) + cons = force_reg (mode, cons); riscv_emit_binary (PLUS, temp1, alt, temp2); emit_insn (gen_rtx_SET (dest, @@ -3986,11 +3988,13 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx cons, rtx alt) rtx temp1 = gen_reg_rtx (mode); rtx temp2 = gen_int_mode (-1 * INTVAL (alt), mode); - /* TEMP2 might not fit into a signed 12 bit immediate suitable - for an addi instruction. If that's the case, force it into - a register. */ + /* TEMP2 and/or ALT might not fit into a signed 12 bit immediate + suitable for an addi instruction. If that's the case, force it + into a register. */ if (!SMALL_OPERAND (INTVAL (temp2))) temp2 = force_reg (mode, temp2); + if (!SMALL_OPERAND (INTVAL (alt))) + alt = force_reg (mode, alt); riscv_emit_binary (PLUS, temp1, cons, temp2); emit_insn (gen_rtx_SET (dest, diff --git a/gcc/testsuite/gcc.target/riscv/zicond-ice-2.c b/gcc/testsuite/gcc.target/riscv/zicond-ice-2.c new file mode 100644 index 000000000000..ffd8dcb5814e --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/zicond-ice-2.c @@ -0,0 +1,11 @@ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gc_zicond -mabi=lp64d" { target { rv64 } } } */ +/* { dg-options "-march=rv32gc_zicond -mabi=ilp32d" { target { rv32 } } } */ + +#define SOME_NUMBER 0x1000 + +unsigned long +d (unsigned long n) +{ + return n > SOME_NUMBER ? SOME_NUMBER : n; +}
From: Tsukasa OI <research_trasio@irq.a4lg.com> Large constant cons and/or alt will trigger ICEs building GCC target libraries (libgomp and libatomic) when the 'Zicond' extension is enabled. For instance, zicond-ice-2.c (new test case in this commit) will cause an ICE when SOME_NUMBER is 0x1000 or larger. While opposite numbers corresponding cons/alt (two temp2 variables) are checked, cons/alt themselves are not checked and causing 2 ICEs building GCC target libraries as of this writing: 1. gcc/libatomic/config/posix/lock.c 2. gcc/libgomp/fortran.c Coercing a large value into a register will fix the issue. gcc/ChangeLog: * config/riscv/riscv.cc (riscv_expand_conditional_move): Force large constant cons/alt into a register. gcc/testsuite/ChangeLog: * gcc.target/riscv/zicond-ice-2.c: New test. This is based on an ICE at libat_lock_n func on gcc/libatomic/config/posix/lock.c but heavily minimized. --- gcc/config/riscv/riscv.cc | 16 ++++++++++------ gcc/testsuite/gcc.target/riscv/zicond-ice-2.c | 11 +++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) create mode 100644 gcc/testsuite/gcc.target/riscv/zicond-ice-2.c base-commit: 78f636d979530c8a649262dbd44914bdfb6f7290