[PATCH] RISC-V: Fix Zicond ICE on large constants

[PATCH] RISC-V: Fix Zicond ICE on large constants

[Tsukasa OI]

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

Re: [PATCH] RISC-V: Fix Zicond ICE on large constants

[Kito Cheng]

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
>

Re: [PATCH] RISC-V: Fix Zicond ICE on large constants

[Jeff Law]

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

Re: [PATCH] RISC-V: Fix Zicond ICE on large constants

[Tsukasa OI]

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

[PATCH v2] RISC-V: Fix Zicond ICE on large constants

[Tsukasa OI]

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.

It also coerce a large cons into a register on "imm, imm" case (the author
could not reproduce but possible to cause an ICE).

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                     | 21 +++++++++++++------
 gcc/testsuite/gcc.target/riscv/zicond-ice-2.c | 11 ++++++++++
 2 files changed, 26 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..e306d57814be 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -3917,6 +3917,11 @@ riscv_expand_conditional_move (rtx dest, rtx op, rtx cons, rtx alt)
 				  gen_rtx_IF_THEN_ELSE (mode, cond,
 							CONST0_RTX (mode),
 							alt)));
+	  /* 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 (cons)))
+	    cons = force_reg (mode, cons);
 	  riscv_emit_binary (PLUS, dest, dest, cons);
 	  return true;
 	}
@@ -3940,11 +3945,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 +3993,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: 72b639760a891c406725854bfb08132c83f0761a
-- 
2.42.0

Re: [PATCH v2] RISC-V: Fix Zicond ICE on large constants

[Jeff Law]

On 9/5/23 06:08, Tsukasa OI 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.
> 
> It also coerce a large cons into a register on "imm, imm" case (the author
> could not reproduce but possible to cause an ICE).
> 
> 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.
"New test." is sufficient.  No need to change it, just a note going forward.

OK.  Thanks for taking care of this.

jeff

Re: [PATCH v2] RISC-V: Fix Zicond ICE on large constants

[Tsukasa OI]

On 2023/09/06 10:22, Jeff Law wrote:
> 
> 
> On 9/5/23 06:08, Tsukasa OI 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.
>>
>> It also coerce a large cons into a register on "imm, imm" case (the
>> author
>> could not reproduce but possible to cause an ICE).
>>
>> 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.
> "New test." is sufficient.  No need to change it, just a note going
> forward.
> 
> OK.  Thanks for taking care of this.
> 
> jeff
> 

Thanks for reviewing.  I'll remember that for the next time (I am adding
brief description in Binutils but not *that* long compared to this).

Committing.

Tsukasa