[PATCH] RISC-V: Reset the length to the default of 4 for FP comparisons

[PATCH] RISC-V: Reset the length to the default of 4 for FP comparisons

[Maciej W. Rozycki]

The default length for floating-point compare operations is overridden 
to 8, however the FEQ.fmt, FLT.fmt, FLE.fmt machine instructions and 
FGE.fmt, FGT.fmt assembly idioms the relevant RTL insns produce are all 
4 bytes long each.  And all the floating-point compare RTL insns that 
produce multiple machine instructions explicitly set their lengths.

Remove the override then, letting the default of 4 apply for the single 
instruction case.

	gcc/
	* config/riscv/riscv.md (length): Remove the explicit setting 
	for "fcmp".
---
Hi,

 So for:

int
feq (float x, float y)
{
  return x == y;
}

we get:

	.globl	feq
	.type	feq, @function
feq:
	feq.s	a0,fa0,fa1	# 15	[c=4 l=8]  *cstoresfdi4
	ret		# 24	[c=0 l=4]  simple_return
	.size	feq, .-feq

which is obviously wrong given:

Disassembly of section .text:

0000000000000000 <feq>:
   0:	a0b52553          	feq.s	a0,fa0,fa1
   4:	8082                	ret

(hmm tabs are odd here too, but that's a binutils issue).  I note that the 
override has always been there since the RISC-V port landed, so I take it 
it's a missed leftover from an earlier situation.

 With the change in place we instead get:

	.globl	feq
	.type	feq, @function
feq:
	feq.s	a0,fa0,fa1	# 15	[c=4 l=4]  *cstoresfdi4
	ret		# 24	[c=0 l=4]  simple_return
	.size	feq, .-feq

which I find so relieving.

 No regressions in the testsuite (and I haven't checked how it affects 
instruction scheduling, especially with `-Os', but I think it's obviously 
correct really).  OK to apply?

  Maciej
---
 gcc/config/riscv/riscv.md |    2 --
 1 file changed, 2 deletions(-)

gcc-riscv-fcmp-length.diff
Index: gcc/gcc/config/riscv/riscv.md
===================================================================
--- gcc.orig/gcc/config/riscv/riscv.md
+++ gcc/gcc/config/riscv/riscv.md
@@ -231,8 +231,6 @@
 
 	  (eq_attr "got" "load") (const_int 8)
 
-	  (eq_attr "type" "fcmp") (const_int 8)
-
 	  ;; SHIFT_SHIFTs are decomposed into two separate instructions.
 	  (eq_attr "move_type" "shift_shift")
 		(const_int 8)

Re: [PATCH] RISC-V: Reset the length to the default of 4 for FP comparisons

[Kito Cheng]

LGTM, *f<quiet_pattern>_quiet<ANYF:mode><X:mode>4_default and
*f<quiet_pattern>_quiet<ANYF:mode><X:mode>4_snan has set their own
length and the only user of this setting is
*cstore<ANYF:mode><X:mode>4, but apparently the length if 4 for that
not 8.

Thanks!

On Thu, Jun 9, 2022 at 9:36 PM Maciej W. Rozycki <macro@embecosm.com> wrote:
>
> The default length for floating-point compare operations is overridden
> to 8, however the FEQ.fmt, FLT.fmt, FLE.fmt machine instructions and
> FGE.fmt, FGT.fmt assembly idioms the relevant RTL insns produce are all
> 4 bytes long each.  And all the floating-point compare RTL insns that
> produce multiple machine instructions explicitly set their lengths.
>
> Remove the override then, letting the default of 4 apply for the single
> instruction case.
>
>         gcc/
>         * config/riscv/riscv.md (length): Remove the explicit setting
>         for "fcmp".
> ---
> Hi,
>
>  So for:
>
> int
> feq (float x, float y)
> {
>   return x == y;
> }
>
> we get:
>
>         .globl  feq
>         .type   feq, @function
> feq:
>         feq.s   a0,fa0,fa1      # 15    [c=4 l=8]  *cstoresfdi4
>         ret             # 24    [c=0 l=4]  simple_return
>         .size   feq, .-feq
>
> which is obviously wrong given:
>
> Disassembly of section .text:
>
> 0000000000000000 <feq>:
>    0:   a0b52553                feq.s   a0,fa0,fa1
>    4:   8082                    ret
>
> (hmm tabs are odd here too, but that's a binutils issue).  I note that the
> override has always been there since the RISC-V port landed, so I take it
> it's a missed leftover from an earlier situation.
>
>  With the change in place we instead get:
>
>         .globl  feq
>         .type   feq, @function
> feq:
>         feq.s   a0,fa0,fa1      # 15    [c=4 l=4]  *cstoresfdi4
>         ret             # 24    [c=0 l=4]  simple_return
>         .size   feq, .-feq
>
> which I find so relieving.
>
>  No regressions in the testsuite (and I haven't checked how it affects
> instruction scheduling, especially with `-Os', but I think it's obviously
> correct really).  OK to apply?
>
>   Maciej
> ---
>  gcc/config/riscv/riscv.md |    2 --
>  1 file changed, 2 deletions(-)
>
> gcc-riscv-fcmp-length.diff
> Index: gcc/gcc/config/riscv/riscv.md
> ===================================================================
> --- gcc.orig/gcc/config/riscv/riscv.md
> +++ gcc/gcc/config/riscv/riscv.md
> @@ -231,8 +231,6 @@
>
>           (eq_attr "got" "load") (const_int 8)
>
> -         (eq_attr "type" "fcmp") (const_int 8)
> -
>           ;; SHIFT_SHIFTs are decomposed into two separate instructions.
>           (eq_attr "move_type" "shift_shift")
>                 (const_int 8)

Re: [PATCH] RISC-V: Reset the length to the default of 4 for FP comparisons

[Maciej W. Rozycki]

On Thu, 9 Jun 2022, Kito Cheng wrote:

> LGTM, *f<quiet_pattern>_quiet<ANYF:mode><X:mode>4_default and
> *f<quiet_pattern>_quiet<ANYF:mode><X:mode>4_snan has set their own
> length and the only user of this setting is
> *cstore<ANYF:mode><X:mode>4, but apparently the length if 4 for that
> not 8.

 I have committed this change now, thank you for your review.

  Maciej