diff mbox series

RISC-V: Allow immediates 17-31 for vector shift.

Message ID 9a28abc7-b305-6ac3-19b7-4426b6c76b43@gmail.com
State Accepted
Headers
Series RISC-V: Allow immediates 17-31 for vector shift. |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Robin Dapp Aug. 18, 2023, 7:37 p.m. UTC 
  Hi,

this patch adds a missing constraint check in order to be able to
print (and not ICE) vector immediates 17-31 for vector shifts.

Regards
 Robin

gcc/ChangeLog:

	* config/riscv/riscv.cc (riscv_print_operand):

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/rvv/autovec/binop/shift-immediate.c: New test.
---
 gcc/config/riscv/riscv.cc                        |  3 ++-
 .../riscv/rvv/autovec/binop/shift-immediate.c    | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/autovec/binop/shift-immediate.c

Comments

Palmer Dabbelt Aug. 18, 2023, 7:56 p.m. UTC | #1 
On Fri, 18 Aug 2023 12:37:06 PDT (-0700), rdapp.gcc@gmail.com wrote:
> Hi,
>
> this patch adds a missing constraint check in order to be able to
> print (and not ICE) vector immediates 17-31 for vector shifts.
>
> Regards
>  Robin
>
> gcc/ChangeLog:
>
> 	* config/riscv/riscv.cc (riscv_print_operand):
>
> gcc/testsuite/ChangeLog:
>
> 	* gcc.target/riscv/rvv/autovec/binop/shift-immediate.c: New test.
> ---
>  gcc/config/riscv/riscv.cc                        |  3 ++-
>  .../riscv/rvv/autovec/binop/shift-immediate.c    | 16 ++++++++++++++++
>  2 files changed, 18 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/autovec/binop/shift-immediate.c
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 49062bef9fc..0f60ffe5f60 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -4954,7 +4954,8 @@ riscv_print_operand (FILE *file, rtx op, int letter)

Looks like the comment at the top of riscv_print_operand() is way out of 
date.  Maybe we should just toss it?

>  	    else if (satisfies_constraint_Wc0 (op))
>  	      asm_fprintf (file, "0");
>  	    else if (satisfies_constraint_vi (op)
> -		     || satisfies_constraint_vj (op))
> +		     || satisfies_constraint_vj (op)
> +		     || satisfies_constraint_vk (op))
>  	      asm_fprintf (file, "%wd", INTVAL (elt));
>  	    else
>  	      output_operand_lossage ("invalid vector constant");
> diff --git a/gcc/testsuite/gcc.target/riscv/rvv/autovec/binop/shift-immediate.c b/gcc/testsuite/gcc.target/riscv/rvv/autovec/binop/shift-immediate.c
> new file mode 100644
> index 00000000000..a2e1c33f4fa
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/rvv/autovec/binop/shift-immediate.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-std=c99 -march=rv32gcv -mabi=ilp32d -O2 --param=riscv-autovec-preference=scalable" } */
> +
> +#define uint8_t unsigned char
> +
> +void foo1 (uint8_t *a)
> +{
> +    uint8_t b = a[0];
> +    int val = 0;
> +
> +    for (int i = 0; i < 4; i++)
> +    {
> +        a[i] = (val & 1) ? (-val) >> 17 : val;
> +        val += b;
> +    }
> +}


Unless I'm missing something it looks like we're missing at least Wc1 as 
well, and maybe a few others?

Either way

Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>

Thanks!
Jeff Law Aug. 18, 2023, 11:13 p.m. UTC | #2 
On 8/18/23 13:56, Palmer Dabbelt wrote:
> On Fri, 18 Aug 2023 12:37:06 PDT (-0700), rdapp.gcc@gmail.com wrote:
>> Hi,
>>
>> this patch adds a missing constraint check in order to be able to
>> print (and not ICE) vector immediates 17-31 for vector shifts.
>>
>> Regards
>>  Robin
>>
>> gcc/ChangeLog:
>>
>>     * config/riscv/riscv.cc (riscv_print_operand):
>>
>> gcc/testsuite/ChangeLog:
>>
>>     * gcc.target/riscv/rvv/autovec/binop/shift-immediate.c: New test.
>> ---
>>  gcc/config/riscv/riscv.cc                        |  3 ++-
>>  .../riscv/rvv/autovec/binop/shift-immediate.c    | 16 ++++++++++++++++
>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>  create mode 100644 
>> gcc/testsuite/gcc.target/riscv/rvv/autovec/binop/shift-immediate.c
>>
>> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> index 49062bef9fc..0f60ffe5f60 100644
>> --- a/gcc/config/riscv/riscv.cc
>> +++ b/gcc/config/riscv/riscv.cc
>> @@ -4954,7 +4954,8 @@ riscv_print_operand (FILE *file, rtx op, int 
>> letter)
> 
> Looks like the comment at the top of riscv_print_operand() is way out of 
> date.  Maybe we should just toss it?
I think it's been out of date on every port I've ever worked on! Folks 
start with good intentions, but nobody seems to keep this up-to-date.

So +1 to removing it if someone wants to. I'll even pre-approve such a 
patch.  I'll also pre-approve a patch which brings it up-to-date if 
that's the direction we want to go.

jeff
Jeff Law Aug. 18, 2023, 11:14 p.m. UTC | #3 
On 8/18/23 13:37, Robin Dapp wrote:
> Hi,
> 
> this patch adds a missing constraint check in order to be able to
> print (and not ICE) vector immediates 17-31 for vector shifts.
> 
> Regards
>   Robin
> 
> gcc/ChangeLog:
> 
> 	* config/riscv/riscv.cc (riscv_print_operand):
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/rvv/autovec/binop/shift-immediate.c: New test.
OK
jeff
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 49062bef9fc..0f60ffe5f60 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -4954,7 +4954,8 @@  riscv_print_operand (FILE *file, rtx op, int letter)
 	    else if (satisfies_constraint_Wc0 (op))
 	      asm_fprintf (file, "0");
 	    else if (satisfies_constraint_vi (op)
-		     || satisfies_constraint_vj (op))
+		     || satisfies_constraint_vj (op)
+		     || satisfies_constraint_vk (op))
 	      asm_fprintf (file, "%wd", INTVAL (elt));
 	    else
 	      output_operand_lossage ("invalid vector constant");
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/autovec/binop/shift-immediate.c b/gcc/testsuite/gcc.target/riscv/rvv/autovec/binop/shift-immediate.c
new file mode 100644
index 00000000000..a2e1c33f4fa
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/autovec/binop/shift-immediate.c
@@ -0,0 +1,16 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-std=c99 -march=rv32gcv -mabi=ilp32d -O2 --param=riscv-autovec-preference=scalable" } */
+
+#define uint8_t unsigned char
+
+void foo1 (uint8_t *a)
+{
+    uint8_t b = a[0];
+    int val = 0;
+
+    for (int i = 0; i < 4; i++)
+    {
+        a[i] = (val & 1) ? (-val) >> 17 : val;
+        val += b;
+    }
+}