[Bug target/110748] New: optimize store of DF 0.0

[Bug target/110748] New: optimize store of DF 0.0

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

            Bug ID: 110748
           Summary: optimize store of DF 0.0
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: vineetg at gcc dot gnu.org
  Target Milestone: ---
            Target: RISC-V

Currently a store of int 0 is optimized by using reg x0.

void zi(int *i) {    *i = 0;    }

-O2 =march=rv64gc

  sw  zero,0(a0)
  ret

However a store of positive DF 0.0 generates 2 insns.

void zd(double *d) { *d = 0.0;  }

  fmv.d.x fa5,zero
  fsd     fa5,0(a0)
  ret

Since +0.0 is all zero bits, this could be generated as an int store
   sw zero, 0(a0) 

This is 1 less insn and avoids the FPU thus overall a win.

This came up when discussing an ICE in anewly proposed pass f-m-o by Manolis.
Turns out that it could be an independent optimization opportunity [1].

[1] https://gcc.gnu.org/pipermail/gcc-patches/2023-July/624935.html

[Bug target/110748] RISC-V: optimize store of DF 0.0

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

Vineet Gupta <vineetg at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-07-20
             Status|UNCONFIRMED                 |ASSIGNED

[Bug target/110748] RISC-V: optimize store of DF 0.0

[kito at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

Kito Cheng <kito at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kito at gcc dot gnu.org

--- Comment #1 from Kito Cheng <kito at gcc dot gnu.org> ---
hmmm, weird, GCC 12 did well but something wrong after GCC 13?

https://godbolt.org/z/ToM1qTxrq

void zd(double *d) { *d = 0.0;  }
void zf(float *f) { *f = 0.0;  }

GCC 12:

zd:
        sd      zero,0(a0)
        ret
zf:
        sw      zero,0(a0)
        ret

GCC 13:
zd:
        fmv.d.x fa5,zero
        fsd     fa5,0(a0)
        ret
zf:
        fmv.s.x fa5,zero
        fsw     fa5,0(a0)
        ret

[Bug target/110748] RISC-V: optimize store of DF 0.0

[kito at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #2 from Kito Cheng <kito at gcc dot gnu.org> ---
And seems we already has such constraint for a while, not sure why GCC 13 did
that, I saw the status has changed to ASSIGNED, so I assume Vineet you are
already spending time on that, so I will just stop there :)

[Bug target/110748] RISC-V: optimize store of DF 0.0

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #3 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
Indeed the constraint already exists

(define_insn "*movdf_hardfloat_rv64"
 [(set (match_operand:DF 0 "nonimmediate_operand" "=f,f,f,m,m,*f,*r, 
*r,*r,*m")
                                                            ^^
       (match_operand:DF 1 "move_operand"         "
f,G,m,f,G,*r,*f,*r*G,*m,*r")
                                                            ^^
 )]

At expand time: gen_movdf() -> riscv_legitimize_move forces a reg, as
reg_or_0_operand () returns false.

Breakpoint 7, riscv_legitimize_move (mode=E_DFmode, dest=0x7ffff6db9af8,
src=0x7ffff6c0c050) at ../../gcc/gcc/config/riscv/riscv.cc:2162
2162    {

(gdb) call debug_rtx(dest)
(mem:DF (reg/v/f:DI 134 [ d ]) [1 *d_2(D)+0 S8 A64])
(gdb) call debug_rtx(src)
(const_double:DF 0.0 [0x0.0p+0])

2232    if (!register_operand (dest, mode) && !reg_or_0_operand (src, mode))
(gdb) n
2257            reg = force_reg (mode, src);
(gdb) 
2258            riscv_emit_move (dest, reg);
(gdb) 
2259            return true;

While for int 0, reg_or_0_operand returns true.

[Bug target/110748] RISC-V: optimize store of DF 0.0

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
aarch64 has the following function to check for +0.0:
```
/* Return TRUE if rtx X is immediate constant 0.0 (but not in Decimal
   Floating Point).  */
bool
aarch64_float_const_zero_rtx_p (rtx x)
{
  /* 0.0 in Decimal Floating Point cannot be represented by #0 or
     zr as our callers expect, so no need to check the actual
     value if X is of Decimal Floating Point type.  */
  if (GET_MODE_CLASS (GET_MODE (x)) == MODE_DECIMAL_FLOAT)
    return false;

  if (REAL_VALUE_MINUS_ZERO (*CONST_DOUBLE_REAL_VALUE (x)))
    return !HONOR_SIGNED_ZEROS (GET_MODE (x));
  return real_equal (CONST_DOUBLE_REAL_VALUE (x), &dconst0);
}
```
Which most likely needed for the new predicate ...

[Bug target/110748] RISC-V: optimize store of DF 0.0

[law at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |law at gcc dot gnu.org

--- Comment #5 from Jeffrey A. Law <law at gcc dot gnu.org> ---
I'd bet it's const_0_operand not allowing CONST_DOUBLE.

The question is what unintended side effects we'd have if we allowed
CONST_DOUBLE 0.0 in const_0_operand.

[Bug target/110748] RISC-V: optimize store of DF 0.0

[palmer at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

palmer at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |palmer at gcc dot gnu.org

--- Comment #6 from palmer at gcc dot gnu.org ---
(In reply to Jeffrey A. Law from comment #5)
> I'd bet it's const_0_operand not allowing CONST_DOUBLE.
> 
> The question is what unintended side effects we'd have if we allowed
> CONST_DOUBLE 0.0 in const_0_operand.

We don't have a architectural 0 register, so we'd probably end up needing to
refactor some stuff.  It's probably smoother to add some sort of
"reg_or_0_or_0f_operand" type predicate, and then convert the floating-point
stuff that takes X registers over to that (at least stores and integer->float
conversions, but maybe some comparisons too?).

[Bug target/110748] RISC-V: optimize store of DF 0.0

[palmer at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #7 from palmer at gcc dot gnu.org ---
(In reply to palmer from comment #6)
> (In reply to Jeffrey A. Law from comment #5)
> > I'd bet it's const_0_operand not allowing CONST_DOUBLE.
> > 
> > The question is what unintended side effects we'd have if we allowed
> > CONST_DOUBLE 0.0 in const_0_operand.
> 
> We don't have a architectural 0 register, so we'd probably end up needing to
> refactor some stuff.  It's probably smoother to add some sort of
> "reg_or_0_or_0f_operand" type predicate, and then convert the floating-point
> stuff that takes X registers over to that (at least stores and
> integer->float conversions, but maybe some comparisons too?).

Should have said "We don't have a architectural 0 floating-point register", we
have x0 (which is why that reg_or_0 stuff shows up).

[Bug target/110748] RISC-V: optimize store of DF 0.0

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #8 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
(In reply to Jeffrey A. Law from comment #5)
> I'd bet it's const_0_operand not allowing CONST_DOUBLE.

Correct.

> The question is what unintended side effects we'd have if we allowed
> CONST_DOUBLE 0.0 in const_0_operand.

Exactly. I had the same concern. 
I do have a hack which creates a new predicate and that seems to do the trick.

+(define_predicate "const_0hf_operand"
+  (and (match_code "const_double")
+       (match_test "op == CONST0_RTX (GET_MODE (op))")))
+
+(define_predicate "reg_or_0_operand_inc_hf"
+  (ior (match_operand 0 "reg_or_0_operand")
+       (match_operand 0 "const_0hf_operand")))

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc

-  if (!register_operand (dest, mode) && !reg_or_0_operand (src, mode))
+  if (!register_operand (dest, mode) && !reg_or_0_operand_inc_hf (src, mode))
     {

And it seems to be generating the desired int 0 for double 0.0.

However to Kito's point, this indeed works in gcc 12 so I first need to bisect
what regressed it in 13.

[Bug target/110748] RISC-V: optimize store of DF 0.0

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #9 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
(In reply to Vineet Gupta from comment #8)
> (In reply to Jeffrey A. Law from comment #5)
> > I'd bet it's const_0_operand not allowing CONST_DOUBLE.
> 
> Correct.
> 
> > The question is what unintended side effects we'd have if we allowed
> > CONST_DOUBLE 0.0 in const_0_operand.
> 
> Exactly. I had the same concern. 

[...]

> However to Kito's point, this indeed works in gcc 12 so I first need to
> bisect what regressed it in 13.

The mystery is solved. Guess what it was my change ef85d150b5963 ("RISC-V:
Enable TARGET_SUPPORTS_WIDE_INT") in gcc-13 cycle which made the booboo.

+            * config/riscv/predicates.md (const_0_operand): Remove
+            const_double.

And I don't recall why I did that part. But I guess reinstating it back won't
be that radical, since it wa sin tree for a while. I'll throw it at full
testsuite to see if there are any fallouts.

[Bug target/110748] RISC-V: optimize store of DF 0.0

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #10 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
The fix for handling +0.0 is posted to list - really trivial.

https://gcc.gnu.org/pipermail/gcc-patches/2023-July/625217.html

[Bug target/110748] RISC-V: optimize store of DF 0.0

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #11 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
There's a variation which can be optimized as well and seems non trivial to
implement

Now it is the negative constant -0.0 to be stored to mem. In bit notation this
has a single sign bit set thus can be optimized using a bseti if rv64gc_zbs.

void znd(double *d) { *d = -0.0; }
void znf(float *f)  { *f = -0.0; }

llvm optim these to

znd(double*):
        bseti   a1, zero, 63
        sd      a1, 0(a0)
        ret

znf(float*):
        lui     a1, 0x80000
        sw      a1, 0(a0)
        ret

While gcc resorts to constant pool for both

        lui     a5,%hi(.LANCHOR0)
        fld     fa5,%lo(.LANCHOR0)(a5)
        fsd     fa5,0(a0)
        ret
        .set    .LANCHOR0,. + 0
.LC0:
        .word   0
        .word   -2147483648

[Bug target/110748] RISC-V: optimize store of DF 0.0

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #12 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
> void znd(double *d) { *d = -0.0; }
> void znf(float *f)  { *f = -0.0; }

The rough approach I'm thinking of is to 

(1) Introduce a constraint for -0.0 and perhaps a predicate as well for
"*movdf_hardfloat_rv64". That way df expander can elide the const pool.

(2) Add a combiner pattern to recog a set of -0.0 to bit set, which would be
automatically optim to BSETI if zbs is passed.

[Bug target/110748] RISC-V: optimize store of DF 0.0

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #13 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
You could also look at how aarch64 implemented a similar thing (not just for
-0.0 but for many other constants too):
r8-2248-ga217096563e356fa03c .

[Bug target/110748] RISC-V: optimize store of DF 0.0

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Vineet Gupta <vineetg@gcc.gnu.org>:

https://gcc.gnu.org/g:ecfa870ff29d979bd2c3d411643b551f2b6915b0

commit r14-2731-gecfa870ff29d979bd2c3d411643b551f2b6915b0
Author: Vineet Gupta <vineetg@rivosinc.com>
Date:   Thu Jul 20 11:15:37 2023 -0700

    RISC-V: optim const DF +0.0 store to mem [PR/110748]

    Fixes: ef85d150b5963 ("RISC-V: Enable TARGET_SUPPORTS_WIDE_INT")

    DF +0.0 is bitwise all zeros so int x0 store to mem can be used to optimize
it.

    void zd(double *) { *d = 0.0; }

    currently:

    | fmv.d.x fa5,zero
    | fsd     fa5,0(a0)
    | ret

    With patch

    | sd      zero,0(a0)
    | ret

    The fix updates predicate const_0_operand() so reg_or_0_operand () now
    includes const_double, enabling movdf expander -> riscv_legitimize_move ()
    to generate below vs. an intermediate set (reg:DF) const_double:DF

    | (insn 6 3 0 2 (set (mem:DF (reg/v/f:DI 134 [ d ])
    |        (const_double:DF 0.0 [0x0.0p+0]))

    This change also enables such insns to be recog() by later passes.
    The md pattern "*movdf_hardfloat_rv64" despite already supporting the
    needed constraints {"m","G"} mem/const 0.0 was failing to match because
    the additional condition check reg_or_0_operand() was failing due to
    missing const_double.

    This failure to recog() was triggering an ICE when testing the in-flight
    f-m-o patches and is how all of this started, but then was deemed to be
    an independent optimization of it's own [1].

    [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-July/624857.html

    Its worthwhile to note all the set peices were already there and working
    up until my own commit mentioned at top regressed the whole thing.

    Ran thru full multilib testsuite and no surprises. There was 1 false
    failure due to random string "lw" appearing in lto build assembler output,
    which is also fixed here.

    gcc/ChangeLog:

            PR target/110748
            * config/riscv/predicates.md (const_0_operand): Add back
            const_double.

    gcc/testsuite/ChangeLog:

            * gcc.target/riscv/pr110748-1.c: New Test.
            * gcc.target/riscv/xtheadfmv-fmv.c: Add '\t' around test
            patterns to avoid random string matches.

    Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>

[Bug target/110748] RISC-V: optimize store of DF 0.0

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #15 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
(In reply to Vineet Gupta from comment #12)
> > void znd(double *d) { *d = -0.0; }
> > void znf(float *f)  { *f = -0.0; }

We need 3 set of changes to get const -0.0 working:

1. Allow expand to generate set mem const_couble -0.0
   - rtx cost adj so compress_float_constant () doesn't force_const_mem ()
   - riscv_legitimize_move to allow -0.0 and not force a reg

2. Allow subsequent passes to recog() this set mem const_double -0.0 
   - Beef up "*movdf_hardfloat_rv64" with additional condition check for -0.0 
   - Add a new constraint for -0.0

3. Add a splitter (for split1, not combine) to generate the int reg

On the branch devel/vineetg/optim-double-const-m0 I have double -0.0 working.

znd:
        li      a5,-1
        slli    a5,a5,63
        sd      a5,0(a0)
        ret

There's currently an ICE for zbs

IRA is undoing the split so the insn with const_int 0x80000000_00000000 doesn't
exist for final pass.

expand
------
(insn 6 3 0 2 (set (mem:DF (reg:DI 135)
        (const_double:DF -0.0 [-0x0.0p+0])) {*movdf_hardfloat_rv64}

split1
-----
(insn 10 3 11 2 (set (reg:DI 136)
        (const_int [0x8000000000000000]))

(insn 11 10 0 2 (set (mem:DF (reg:DI 135)
        (subreg:DF (reg:DI 136) 0))

ira
----
(insn 11 9 12 2 (set (mem:DF (reg:DI 135)
        (const_double:DF -0.0 [-0x0.0p+0])) {*movdf_hardfloat_rv64}

[Bug target/110748] RISC-V: optimize store of DF 0.0

[vineetg at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110748

--- Comment #16 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
(In reply to Vineet Gupta from comment #15)

> On the branch devel/vineetg/optim-double-const-m0 I have double -0.0 working.
> 
> znd:
>         li      a5,-1
>         slli    a5,a5,63
>         sd      a5,0(a0)
>         ret
> 
> There's currently an ICE for zbs
> 
> IRA is undoing the split so the insn with const_int 0x80000000_00000000
> doesn't exist for final pass.
> 
> expand
> ------
> (insn 6 3 0 2 (set (mem:DF (reg:DI 135)
>         (const_double:DF -0.0 [-0x0.0p+0])) {*movdf_hardfloat_rv64}
> 
> split1
> -----
> (insn 10 3 11 2 (set (reg:DI 136)
>         (const_int [0x8000000000000000]))
> 
> (insn 11 10 0 2 (set (mem:DF (reg:DI 135)
>         (subreg:DF (reg:DI 136) 0))
> 
> ira
> ----
> (insn 11 9 12 2 (set (mem:DF (reg:DI 135)
>         (const_double:DF -0.0 [-0x0.0p+0])) {*movdf_hardfloat_rv64}

So IRA is doing the equivalent replacement for a register which is referenced
exactly twice: set once and used once, w/o any reg pressure considerations [1].

[1] https://gcc.gnu.org/pipermail/gcc-patches/2023-August/627212.html

There seems to be no easy way around it.