[v3] RISC-V: Fixbug for fsflags instruction error using immediate.

  The pattern mistakenly believes that fsflags can use immediate numbers,
but in fact it does not support it. Immediate numbers should use fsflagsi.

For example:
__builtin_riscv_fsflags(4);

The following error occurred.
/tmp/ccoWdWqT.s: Assembler messages:
/tmp/ccoWdWqT.s:14: Error: illegal operands `fsflags 4'

gcc/ChangeLog:

	* config/riscv/riscv.md: Likewise.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/fsflags.c: New test.
---
 gcc/config/riscv/riscv.md                |  4 ++--
 gcc/testsuite/gcc.target/riscv/fsflags.c | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/fsflags.c
  

-  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSCSR)]
+  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "r")] UNSPECV_FSCSR)]

If you don't allow immediate value in range 0 ~ 31, it should be "register_operand" instead of "csr_operand".



juzhe.zhong@rivai.ai
 
From: Jin Ma
Date: 2023-07-26 10:17
To: gcc-patches
CC: jeffreyalaw; palmer; richard.sandiford; kito.cheng; philipp.tomsich; christoph.muellner; rdapp.gcc; juzhe.zhong; jinma.contrib; Jin Ma
Subject: [PATCH v3] RISC-V: Fixbug for fsflags instruction error using immediate.
The pattern mistakenly believes that fsflags can use immediate numbers,
but in fact it does not support it. Immediate numbers should use fsflagsi.
 
For example:
__builtin_riscv_fsflags(4);
 
The following error occurred.
/tmp/ccoWdWqT.s: Assembler messages:
/tmp/ccoWdWqT.s:14: Error: illegal operands `fsflags 4'
 
gcc/ChangeLog:
 
* config/riscv/riscv.md: Likewise.
 
gcc/testsuite/ChangeLog:
 
* gcc.target/riscv/fsflags.c: New test.
---
gcc/config/riscv/riscv.md                |  4 ++--
gcc/testsuite/gcc.target/riscv/fsflags.c | 16 ++++++++++++++++
2 files changed, 18 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/fsflags.c
 
diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index 4615e811947..74ff9ccc968 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -3074,7 +3074,7 @@ (define_insn "riscv_frcsr"
   "frcsr\t%0")
(define_insn "riscv_fscsr"
-  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSCSR)]
+  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "r")] UNSPECV_FSCSR)]
   "TARGET_HARD_FLOAT || TARGET_ZFINX"
   "fscsr\t%0")
@@ -3087,7 +3087,7 @@ (define_insn "riscv_frflags"
(define_insn "riscv_fsflags"
   [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSFLAGS)]
   "TARGET_HARD_FLOAT || TARGET_ZFINX"
-  "fsflags\t%0")
+  "fsflags%i0\t%0")
(define_insn "*riscv_fsnvsnan<mode>2"
   [(unspec_volatile [(match_operand:ANYF 0 "register_operand" "f")
diff --git a/gcc/testsuite/gcc.target/riscv/fsflags.c b/gcc/testsuite/gcc.target/riscv/fsflags.c
new file mode 100644
index 00000000000..74a97b8a7c7
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/fsflags.c
@@ -0,0 +1,16 @@
+/* Verify that fsflags is using the correct register or immediate.  */
+/* { dg-do compile } */
+/* { dg-require-effective-target hard_float } */
+/* { dg-options "-O" } */
+
+void foo1 (int a)
+{
+   __builtin_riscv_fsflags(a);
+}
+void foo2 ()
+{
+   __builtin_riscv_fsflags(4);
+}
+
+/* { dg-final { scan-assembler-times "fsflags\t" 1 } } */
+/* { dg-final { scan-assembler-times "fsflagsi\t" 1 } } */
-- 
2.17.1
  

> -  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSCSR)]
> +  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "r")] UNSPECV_FSCSR)]
> 
> If you don't allow immediate value in range 0 ~ 31, it should be "register_operand" instead of "csr_operand".
> 
> 

I think directives that support the immediate pattern might be better, on the one
hand fsflagsi are supported in the manual, on the other hand fsflagsi can be
slightly faster than fsflags.

Regards
Jin

> 
> juzhe.zhong@rivai.ai
>
  

I don't understand:
 (define_insn "riscv_fscsr"
-  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSCSR)]
+  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "r")] UNSPECV_FSCSR)]
   "TARGET_HARD_FLOAT || TARGET_ZFINX"
   "fscsr\t%0")

This pattern never allows immediate in the constraint. Why still make predicate allow immediate?
 



juzhe.zhong@rivai.ai
 
From: Jin Ma
Date: 2023-07-26 11:33
To: gcc-patches; juzhe.zhong@rivai.ai
CC: jeffreyalaw; palmer; richard.sandiford; kito.cheng; philipp.tomsich; christoph.muellner; Robin Dapp; jinma.contrib
Subject: Re: [PATCH v3] RISC-V: Fixbug for fsflags instruction error using immediate.
> -  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSCSR)]
> +  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "r")] UNSPECV_FSCSR)]
> 
> If you don't allow immediate value in range 0 ~ 31, it should be "register_operand" instead of "csr_operand".
> 
> 
 
I think directives that support the immediate pattern might be better, on the one
hand fsflagsi are supported in the manual, on the other hand fsflagsi can be
slightly faster than fsflags.
 
Regards
Jin
 
> 
> juzhe.zhong@rivai.ai
>
  

So I guess you should change `fscsr` to `fscsr%i0` instead of dropping
K from the constraint list?

On Wed, Jul 26, 2023 at 11:42 AM juzhe.zhong@rivai.ai
<juzhe.zhong@rivai.ai> wrote:
>
> I don't understand:
>  (define_insn "riscv_fscsr"
> -  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSCSR)]
> +  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "r")] UNSPECV_FSCSR)]
>    "TARGET_HARD_FLOAT || TARGET_ZFINX"
>    "fscsr\t%0")
>
> This pattern never allows immediate in the constraint. Why still make predicate allow immediate?
>
>
>
>
> juzhe.zhong@rivai.ai
>
> From: Jin Ma
> Date: 2023-07-26 11:33
> To: gcc-patches; juzhe.zhong@rivai.ai
> CC: jeffreyalaw; palmer; richard.sandiford; kito.cheng; philipp.tomsich; christoph.muellner; Robin Dapp; jinma.contrib
> Subject: Re: [PATCH v3] RISC-V: Fixbug for fsflags instruction error using immediate.
> > -  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSCSR)]
> > +  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "r")] UNSPECV_FSCSR)]
> >
> > If you don't allow immediate value in range 0 ~ 31, it should be "register_operand" instead of "csr_operand".
> >
> >
>
> I think directives that support the immediate pattern might be better, on the one
> hand fsflagsi are supported in the manual, on the other hand fsflagsi can be
> slightly faster than fsflags.
>
> Regards
> Jin
>
> >
> > juzhe.zhong@rivai.ai
> >
  

Yes. I agree.

I didn't take a look into SPEC. Not sure whether fcsr has immediate form.

I mean this patch change in 'fcsr' is quite confusing.

You should either fix the assembly code-gen if fcsr has immediate form,

or fix predicate and constraint both (should not fix constraint only).

Thanks.


juzhe.zhong@rivai.ai
 
From: Kito Cheng
Date: 2023-07-26 11:45
To: juzhe.zhong@rivai.ai
CC: jinma; gcc-patches; jeffreyalaw; palmer; richard.sandiford; philipp.tomsich; christoph.muellner; Robin Dapp; jinma.contrib
Subject: Re: Re: [PATCH v3] RISC-V: Fixbug for fsflags instruction error using immediate.
So I guess you should change `fscsr` to `fscsr%i0` instead of dropping
K from the constraint list?
 
On Wed, Jul 26, 2023 at 11:42 AM juzhe.zhong@rivai.ai
<juzhe.zhong@rivai.ai> wrote:
>
> I don't understand:
>  (define_insn "riscv_fscsr"
> -  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSCSR)]
> +  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "r")] UNSPECV_FSCSR)]
>    "TARGET_HARD_FLOAT || TARGET_ZFINX"
>    "fscsr\t%0")
>
> This pattern never allows immediate in the constraint. Why still make predicate allow immediate?
>
>
>
>
> juzhe.zhong@rivai.ai
>
> From: Jin Ma
> Date: 2023-07-26 11:33
> To: gcc-patches; juzhe.zhong@rivai.ai
> CC: jeffreyalaw; palmer; richard.sandiford; kito.cheng; philipp.tomsich; christoph.muellner; Robin Dapp; jinma.contrib
> Subject: Re: [PATCH v3] RISC-V: Fixbug for fsflags instruction error using immediate.
> > -  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSCSR)]
> > +  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "r")] UNSPECV_FSCSR)]
> >
> > If you don't allow immediate value in range 0 ~ 31, it should be "register_operand" instead of "csr_operand".
> >
> >
>
> I think directives that support the immediate pattern might be better, on the one
> hand fsflagsi are supported in the manual, on the other hand fsflagsi can be
> slightly faster than fsflags.
>
> Regards
> Jin
>
> >
> > juzhe.zhong@rivai.ai
> >
  

> So I guess you should change `fscsr` to `fscsr%i0` instead of dropping
> K from the constraint list?
> 
Sorry, you are right. I thought you were talking about fsflags, 
but I didn't notice it was fscsr. I'll correct it right away.
> On Wed, Jul 26, 2023 at 11:42 AM juzhe.zhong@rivai.ai
> <juzhe.zhong@rivai.ai> wrote:
> >
> > I don't understand:
> >  (define_insn "riscv_fscsr"
> > -  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSCSR)]
> > +  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "r")] UNSPECV_FSCSR)]
> >    "TARGET_HARD_FLOAT || TARGET_ZFINX"
> >    "fscsr\t%0")
> >
> > This pattern never allows immediate in the constraint. Why still make predicate allow immediate?
> >
> >
> >
> >
> > juzhe.zhong@rivai.ai
> >
> > From: Jin Ma
> > Date: 2023-07-26 11:33
> > To: gcc-patches; juzhe.zhong@rivai.ai
> > CC: jeffreyalaw; palmer; richard.sandiford; kito.cheng; philipp.tomsich; christoph.muellner; Robin Dapp; jinma.contrib
> > Subject: Re: [PATCH v3] RISC-V: Fixbug for fsflags instruction error using immediate.
> > > -  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSCSR)]
> > > +  [(unspec_volatile [(match_operand:SI 0 "csr_operand" "r")] UNSPECV_FSCSR)]
> > >
> > > If you don't allow immediate value in range 0 ~ 31, it should be "register_operand" instead of "csr_operand".
> > >
> > >
> >
> > I think directives that support the immediate pattern might be better, on the one
> > hand fsflagsi are supported in the manual, on the other hand fsflagsi can be
> > slightly faster than fsflags.
> >
> > Regards
> > Jin
> >
> > >
> > > juzhe.zhong@rivai.ai
> > >