[Bug other/115687] New: RISC-V optimization when "lui" instructions can be merged

[Bug other/115687] New: RISC-V optimization when "lui" instructions can be merged

[Explorer09 at gmail dot com]

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

            Bug ID: 115687
           Summary: RISC-V optimization when "lui" instructions can be
                    merged
           Product: gcc
           Version: 14.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: other
          Assignee: unassigned at gcc dot gnu.org
          Reporter: Explorer09 at gmail dot com
  Target Milestone: ---

```c
#include <stdio.h>
int main() {
    unsigned int a = 0x4010; // (0x4 << 12) + 0x10
    unsigned int b = 0x4020; // (0x4 << 12) + 0x20
    unsigned int c = 0x3FF0; // (0x4 << 12) - 0x10
    printf("0x%08x 0x%08x 0x%08x\n", a, b, c);
    return 0;
}
```

RISC-V (64-bits) GCC 14.1 with -Os option produces:

```text
    li a3,16384
    li a2,16384
    li a1,16384
    lui a0,%hi(.LC0)
    addi sp,sp,-16
    addi a3,a3,-16
    addi a2,a2,32
    addi a1,a1,16
    addi a0,a0,%lo(.LC0)
    sd ra,8(sp)
    call printf
    ld ra,8(sp)
    li a0,0
    addi sp,sp,16
    jr ra
```

There are three "li ...,16384" instructions generated, while it is possible to
reduce them to one. For example, by using the same "a1" register for the
addends:

```text
    li a1,16384
    ...
    addi a3,a1,-16
    addi a2,a1,32
    addi a1,a1,16
```

[Bug target/115687] RISC-V optimization when "lui" instructions can be merged

[palmer at gcc dot gnu.org]

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

palmer at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2024-06-27
     Ever confirmed|0                           |1
                 CC|                            |palmer at gcc dot gnu.org
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from palmer at gcc dot gnu.org ---
Looks reasonable to me.  Here's a slightly smaller test case

long bar(unsigned int a, unsigned int b, unsigned int c);

long foo(void) {
    unsigned int a = 0x4010; // (0x4 << 12) + 0x10
    unsigned int b = 0x4020; // (0x4 << 12) + 0x20
    unsigned int c = 0x3FF0; // (0x4 << 12) - 0x10
    return bar(a, b, c);
}

which under -O2 produces

        li      a2,16384
        li      a1,16384
        li      a0,16384
        addi    a2,a2,-16
        addi    a1,a1,32
        addi    a0,a0,16
        tail    bar

Jeff had been doing a bunch of constant generation stuff, not sure if he's got
a fix for this one in the works?

[Bug target/115687] RISC-V optimization when "lui" instructions can be merged

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
There is some code in cse.cc which does handle this.
See
https://gcc.gnu.org/onlinedocs/gccint/Misc.html#index-TARGET_005fCONST_005fANCHOR
also.

[Bug target/115687] RISC-V optimization when "lui" instructions can be merged

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #2)
> There is some code in cse.cc which does handle this.
> See
> https://gcc.gnu.org/onlinedocs/gccint/Misc.html#index-
> TARGET_005fCONST_005fANCHOR also.

MIPS, aarch64 and rs6000 all define TARGET_CONST_ANCHOR (well MIPS sets
targetm.const_anchor depending on if it is mips16/micromips or mips32/64).

[Bug target/115687] RISC-V optimization when "lui" instructions can be merged

[palmer at gcc dot gnu.org]

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

--- Comment #4 from palmer at gcc dot gnu.org ---
Just poking around a bit: I think this is coming from CSE, which is replacing

(insn 5 2 6 2 (set (reg:DI 135)
        (const_int 16384 [0x4000])) "pr115687.c":7:12 275 {*movdi_64bit}
     (nil))
(insn 6 5 7 2 (set (reg:DI 12 a2)
        (plus:DI (reg:DI 135)
            (const_int -16 [0xfffffffffffffff0]))) "pr115687.c":7:12 5 {adddi3}
     (expr_list:REG_EQUAL (const_int 16368 [0x3ff0])
        (nil)))
(insn 7 6 8 2 (set (reg:DI 136)
        (const_int 16384 [0x4000])) "pr115687.c":7:12 275 {*movdi_64bit}
     (nil))
(insn 8 7 9 2 (set (reg:DI 11 a1)
        (plus:DI (reg:DI 136)
            (const_int 32 [0x20]))) "pr115687.c":7:12 5 {adddi3}
     (expr_list:REG_EQUAL (const_int 16416 [0x4020])
        (nil)))
(insn 9 8 10 2 (set (reg:DI 137)
        (const_int 16384 [0x4000])) "pr115687.c":7:12 275 {*movdi_64bit}
     (nil))

with

(insn 5 2 6 2 (set (reg:DI 135)
        (const_int 16384 [0x4000])) "pr115687.c":7:12 275 {*movdi_64bit}
     (nil))
(insn 6 5 7 2 (set (reg:DI 12 a2)
        (const_int 16368 [0x3ff0])) "pr115687.c":7:12 273 {*mvconst_internal}
     (expr_list:REG_DEAD (reg:DI 135)
        (expr_list:REG_EQUAL (const_int 16368 [0x3ff0])
            (nil))))
(insn 7 6 8 2 (set (reg:DI 136)
        (reg:DI 135)) "pr115687.c":7:12 275 {*movdi_64bit}
     (expr_list:REG_EQUAL (const_int 16384 [0x4000])
        (nil)))
(insn 8 7 9 2 (set (reg:DI 11 a1)
        (const_int 16416 [0x4020])) "pr115687.c":7:12 273 {*mvconst_internal}
     (expr_list:REG_DEAD (reg:DI 136)
        (expr_list:REG_EQUAL (const_int 16416 [0x4020])
            (nil))))
(insn 9 8 10 2 (set (reg:DI 137)
        (reg:DI 135)) "pr115687.c":7:12 275 {*movdi_64bit}
     (expr_list:REG_EQUAL (const_int 16384 [0x4000])
        (nil)))
(insn 10 9 11 2 (set (reg:DI 10 a0)
        (const_int 16400 [0x4010])) "pr115687.c":7:12 273 {*mvconst_internal}
     (expr_list:REG_DEAD (reg:DI 137)
        (expr_list:REG_EQUAL (const_int 16400 [0x4010])
            (nil))))

that seems to be as-designed -- or at least as this comment in cse.cc seems to
be describing

          /* Find cheapest and skip it for the next time.   For items
             of equal cost, use this order:
             src_folded, src, src_eqv, src_related and hash table entry.  */

That seems like a bit of a heuristic, but I haven't poked around this stuff to
really understand how it's handling multiple uses of the incoming constant
anchor.

[Bug target/115687] RISC-V optimization when "lui" instructions can be merged

[palmer at gcc dot gnu.org]

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

--- Comment #5 from palmer at gcc dot gnu.org ---
(In reply to Andrew Pinski from comment #3)
> (In reply to Andrew Pinski from comment #2)
> > There is some code in cse.cc which does handle this.
> > See
> > https://gcc.gnu.org/onlinedocs/gccint/Misc.html#index-
> > TARGET_005fCONST_005fANCHOR also.
> 
> MIPS, aarch64 and rs6000 all define TARGET_CONST_ANCHOR (well MIPS sets
> targetm.const_anchor depending on if it is mips16/micromips or mips32/64).

Oh, thanks, I didn't know about that.  It looks like just adding 

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 9bba5da016e..6080298c36c 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -12019,6 +12019,9 @@ riscv_c_mode_for_floating_type (enum tree_index ti)
 #undef TARGET_C_MODE_FOR_FLOATING_TYPE
 #define TARGET_C_MODE_FOR_FLOATING_TYPE riscv_c_mode_for_floating_type

+#undef TARGET_CONST_ANCHOR
+#define TARGET_CONST_ANCHOR 0x4000
+
 struct gcc_target targetm = TARGET_INITIALIZER;

 #include "gt-riscv.h"

isn't enough for us here, but it seems like we should have something along
those lines.  2d7c73ee5ea ("AArch64: Enable TARGET_CONST_ANCHOR") has a test
case, so hopefully it's not that tricky to get something that exposes the issue
on RISC-V as well...

[Bug target/115687] RISC-V optimization when "lui" instructions can be merged

[andrew at sifive dot com]

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

Andrew Waterman <andrew at sifive dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |andrew at sifive dot com

--- Comment #6 from Andrew Waterman <andrew at sifive dot com> ---
I note MIPS sets TARGET_CONST_ANCHOR to 0x8000, and that architecture's ADDIU
instruction has a 16-bit immediate.  RISC-V's ADDI instruction has a 12-bit
immediate, so presumably we should be setting it to 0x800.

[Bug target/115687] RISC-V optimization when "lui" instructions can be merged

[vineetg at gcc dot gnu.org]

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

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

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

--- Comment #7 from Vineet Gupta <vineetg at gcc dot gnu.org> ---
Not sure if the anchor fixes the issue.

The deeper issue with CSE1 tripping up is the unfortunate side-effect of
mvconst_internal splitter, added in gcc-13 cycle.

commit 2e886eef7f2b5aadb00171af868f0895b647c3a4
Author: Raphael Moreira Zinsly <rzinsly@ventanamicro.com>
Date:   Tue Dec 27 18:29:25 2022 -0500

    RISC-V: Produce better code with complex constants [PR95632] [PR106602]

    gcc/Changelog:
            PR target/95632
            PR target/106602
            * config/riscv/riscv.md: New pattern to simulate complex
            const_int loads.

It helped fix a bunch of issues but with time both Jeff and I agree that it
needs to be reverted. Its on our list of TODO - RISE CT_00_029.

With that reverted we only get single copy of LUI.

[Bug target/115687] RISC-V optimization when "lui" instructions can be merged

[palmer at gcc dot gnu.org]

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

--- Comment #8 from Palmer Dabbelt <palmer at gcc dot gnu.org> ---
(In reply to Andrew Waterman from comment #6)
> I note MIPS sets TARGET_CONST_ANCHOR to 0x8000, and that architecture's
> ADDIU instruction has a 16-bit immediate.  RISC-V's ADDI instruction has a
> 12-bit immediate, so presumably we should be setting it to 0x800.

Ya, sorry, I wasn't paying attention -- regardless I think Vineet's on the
right track here with the splitter messing with us here, the incoming code has
a constant anchor already so dealing with those is sort of a different problem.

If removing that splitter is on the TODO list that seems reasonable, though I'd
taken a very different approach and just hacked up a post-split CSE as it seems
like we could end up in more situations like this.  I have no idea if that's a
sane idea, I sent an RFC to the lists to see what people think...