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

[Kito Cheng <kito.cheng@gmail.com>]

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)