RISC-V: Fix Zicond ICE on large constants

  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
  

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