[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[syq at gcc dot gnu.org]

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

YunQiang Su <syq at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2024-05-22
                 CC|                            |syq at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1

--- Comment #5 from YunQiang Su <syq at gcc dot gnu.org> ---
Assign it to me.

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[syq at gcc dot gnu.org]

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

--- Comment #6 from YunQiang Su <syq at gcc dot gnu.org> ---
The attached patch cannot work now.

It is not correct, and it happened work due to good luck that the same register
was allocated for these 2 instructions.

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[mschiffer@universe-factory.net]

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

--- Comment #7 from Matthias Schiffer <mschiffer@universe-factory.net> ---
(In reply to YunQiang Su from comment #6)
> The attached patch cannot work now.
> 
> It is not correct, and it happened work due to good luck that the same
> register was allocated for these 2 instructions.

I believe this is not the case. The gp init sequence is inserted very late, and
no register allocation is involved - the use of registers $2 and $3 is
hardcoded:
https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/mips/mips.cc;h=b63d40a357b7c1f294e2c82062f0ef75fc307ba8;hb=HEAD#l12164

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[syq at gcc dot gnu.org]

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

--- Comment #8 from YunQiang Su <syq at gcc dot gnu.org> ---
Ohh, In fact we should use $28 if TARGET_USE_GOT.

Can you help to test this patch?

```
diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
index b63d40a357b..fe8641d3916 100644
--- a/gcc/config/mips/mips.cc
+++ b/gcc/config/mips/mips.cc
@@ -3342,7 +3342,7 @@ mips16_gp_pseudo_reg (void)
 rtx
 mips_pic_base_register (rtx temp)
 {
-  if (MIPS16_GP_LOADS ||!TARGET_MIPS16)
+  if (MIPS16_GP_LOADS || TARGET_USE_GOT ||!TARGET_MIPS16)
     return pic_offset_table_rtx;

   if (currently_expanding_to_rtl)
```

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[syq at gcc dot gnu.org]

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

--- Comment #9 from YunQiang Su <syq at gcc dot gnu.org> ---
(In reply to Matthias Schiffer from comment #7)
> (In reply to YunQiang Su from comment #6)
> > The attached patch cannot work now.
> > 
> > It is not correct, and it happened work due to good luck that the same
> > register was allocated for these 2 instructions.
> 
> I believe this is not the case. The gp init sequence is inserted very late,
> and no register allocation is involved - the use of registers $2 and $3 is
> hardcoded:

Here $6(a2) is hardcoded, while $3(v1) is not.

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[mschiffer@universe-factory.net]

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

--- Comment #10 from Matthias Schiffer <mschiffer@universe-factory.net> ---
(In reply to YunQiang Su from comment #8)
> Ohh, In fact we should use $28 if TARGET_USE_GOT.
> 
> Can you help to test this patch?
> 
> ```
> diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> index b63d40a357b..fe8641d3916 100644
> --- a/gcc/config/mips/mips.cc
> +++ b/gcc/config/mips/mips.cc
> @@ -3342,7 +3342,7 @@ mips16_gp_pseudo_reg (void)
>  rtx
>  mips_pic_base_register (rtx temp)
>  {
> -  if (MIPS16_GP_LOADS ||!TARGET_MIPS16)
> +  if (MIPS16_GP_LOADS || TARGET_USE_GOT ||!TARGET_MIPS16)
>      return pic_offset_table_rtx;
>  
>    if (currently_expanding_to_rtl)
> ```

Testing might take a while, I haven't built GCC for some time.

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[syq at gcc dot gnu.org]

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

--- Comment #11 from YunQiang Su <syq at gcc dot gnu.org> ---
(In reply to YunQiang Su from comment #8)
> Ohh, In fact we should use $28 if TARGET_USE_GOT.
> 
> Can you help to test this patch?
> 
> ```
> diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> index b63d40a357b..fe8641d3916 100644
> --- a/gcc/config/mips/mips.cc
> +++ b/gcc/config/mips/mips.cc
> @@ -3342,7 +3342,7 @@ mips16_gp_pseudo_reg (void)
>  rtx
>  mips_pic_base_register (rtx temp)
>  {
> -  if (MIPS16_GP_LOADS ||!TARGET_MIPS16)
> +  if (MIPS16_GP_LOADS || TARGET_USE_GOT ||!TARGET_MIPS16)
>      return pic_offset_table_rtx;
>  
>    if (currently_expanding_to_rtl)
> ```

This patch can trigger some ICE....

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[syq at gcc dot gnu.org]

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

--- Comment #12 from YunQiang Su <syq at gcc dot gnu.org> ---
You are right: the decision to use $6 is too late.
So let's force to use it in expand pass.

```
diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
index b63d40a357b..84ff29cd62b 100644
--- a/gcc/config/mips/mips.cc
+++ b/gcc/config/mips/mips.cc
@@ -3318,7 +3318,11 @@ mips16_gp_pseudo_reg (void)
     {
       rtx_insn *scan;

-      cfun->machine->mips16_gp_pseudo_rtx = gen_reg_rtx (Pmode);
+      if (TARGET_USE_GOT)
+       cfun->machine->mips16_gp_pseudo_rtx
+               = gen_rtx_REG (Pmode, POST_CALL_TMP_REG);
+      else
+       cfun->machine->mips16_gp_pseudo_rtx = gen_reg_rtx (Pmode);

       push_topmost_sequence ();


```

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[mschiffer@universe-factory.net]

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

--- Comment #13 from Matthias Schiffer <mschiffer@universe-factory.net> ---
I don't think the register used matters -  changing it may hide the bug in
specific instances, but it does not fix the root cause.

I've now built a simpler reproducer which still seems to exhibit the same issue
with your latest patch (however I've only built a baremetal GCC with your patch
and looked at the generated code, I've not actually run this example on the
affected platforms - I might be overlooking something. Will try to get a full
toolchain build in the next days).

The basic premise of the following code:

In test(), the return value `ret` must be moved from v0 to a different register
temporarily for calling foo(). Using the inline asm, GCC is nudged to use v1 as
this temporary register.

As GCC knows the contents of foo() and bar(), it assumes that the value of v1
is preserved across the call to foo(). This assumption is wrong because the gp
setup code is inserted at the beginning of bar after all optimization and
register allocation has already happened. As mentioned before, this setup code
clobbers v1.

```
unsigned ext(void);

__attribute__((noinline))
static void foo(void) {
        /* Do not let the optimizer remove foo and bar */
        asm volatile("");
}

__attribute__((noinline))
static void bar(void) {
        foo();
}

unsigned test(void)
{
        unsigned ret = ext();

        register unsigned v1 asm("v1") = ret;
        asm volatile("" :: "r"(v1));

        bar();

        return ret;
}
```

`objdump -d -r` output (built using GCC commit
05daf617ea22e1d818295ed2d037456937e23530, with "-Os -mips32r2 -mtune=24kc
-mabicalls -mips16 -fpic"):

```
Disassembly of section .text:

00000000 <foo>:
   0:   e8a0            jrc     ra
   2:   6500            nop

00000004 <bar>:
   4:   f000 6a00       li      v0,0
                        4: R_MIPS16_HI16        _gp_disp
   8:   f000 0b00       la      v1,8 <bar+0x4>
                        8: R_MIPS16_LO16        _gp_disp
   c:   f400 3240       sll     v0,16
  10:   e269            addu    v0,v1
  12:   64c4            save    32,ra
  14:   659a            move    gp,v0
  16:   d204            sw      v0,16(sp)
  18:   675c            move    v0,gp
  1a:   f000 9a40       lw      v0,0(v0)
                        1a: R_MIPS16_GOT16      foo
  1e:   f000 4a00       addiu   v0,0
                        1e: R_MIPS16_LO16       foo
  22:   ea40            jalr    v0
  24:   653a            move    t9,v0
  26:   6444            restore 32,ra
  28:   e8a0            jrc     ra
  2a:   6500            nop

0000002c <test>:
  2c:   f000 6a00       li      v0,0
                        2c: R_MIPS16_HI16       _gp_disp
  30:   f000 0b00       la      v1,30 <test+0x4>
                        30: R_MIPS16_LO16       _gp_disp
  34:   f400 3240       sll     v0,16
  38:   e269            addu    v0,v1
  3a:   659a            move    gp,v0
  3c:   64e4            save    32,ra,s0
  3e:   671c            move    s0,gp
  40:   d204            sw      v0,16(sp)
  42:   f000 9840       lw      v0,0(s0)
                        42: R_MIPS16_CALL16     ext
  46:   ea40            jalr    v0
  48:   653a            move    t9,v0
  4a:   6762            move    v1,v0
  4c:   f000 9800       lw      s0,0(s0)
                        4c: R_MIPS16_GOT16      bar
  50:   f000 4800       addiu   s0,0
                        50: R_MIPS16_LO16       bar
  54:   e840            jalr    s0
  56:   6538            move    t9,s0
  58:   6464            restore 32,ra,s0
  5a:   e820            jr      ra
  5c:   6743            move    v0,v1
  5e:   6500            nop
```

At 4a, the return value is moved to v1. At 5c, it is supposed to be moved back,
but v1 has been clobbered in the mean time.

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[syq at gcc dot gnu.org]

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

--- Comment #14 from YunQiang Su <syq at gcc dot gnu.org> ---
Ohh, sorry for my misunderstanding. Your patch is correct.

The real problem is that, $3 is used by `mips_output_function_prologue`,
which is the final for output asm source code, and thus the IRA pass
cannot be aware that $3 is used.


So we have to emit some clobbers before IRA.

We have 2 choice:

1. Your choice, aka emit clobbers just before the the call function
2. the entrance every function that need to use GP

@@ -3329,6 +3331,8 @@ mips16_gp_pseudo_reg (void)
       rtx set = gen_load_const_gp (cfun->machine->mips16_gp_pseudo_rtx);
       rtx_insn *insn = emit_insn_after (set, scan);
       INSN_LOCATION (insn) = 0;
+      emit_clobber (MIPS16_PIC_TEMP);
+      emit_clobber (MIPS_PROLOGUE_TEMP (Pmode));

       pop_topmost_sequence ();
     }

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[cvs-commit at gcc dot gnu.org]

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

--- Comment #15 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by YunQiang Su <syq@gcc.gnu.org>:

https://gcc.gnu.org/g:915440eed21de367cb41857afb5273aff5bcb737

commit r15-911-g915440eed21de367cb41857afb5273aff5bcb737
Author: YunQiang Su <syq@gcc.gnu.org>
Date:   Wed May 29 02:28:25 2024 +0800

    MIPS16: Mark $2/$3 as clobbered if GP is used

    PR Target/84790.
    The gp init sequence
            li      $2,%hi(_gp_disp)
            addiu   $3,$pc,%lo(_gp_disp)
            sll     $2,16
            addu    $2,$3
    is generated directly in `mips_output_function_prologue`, and does
    not appear in the RTL.

    So the IRA/IPA passes are not aware that $2/$3 have been clobbered,
    so they may be used for cross (local) function call.

    Let's mark $2/$3 clobber both:
      - Just after the UNSPEC_GP RTL of a function;
      - Just after a function call.

    Reported-by: Matthias Schiffer <mschiffer@universe-factory.net>
    Origin-Patch-by: Felix Fietkau <nbd@nbd.name>.

    gcc
            * config/mips/mips.cc(mips16_gp_pseudo_reg): Mark
            MIPS16_PIC_TEMP and MIPS_PROLOGUE_TEMP clobbered.
            (mips_emit_call_insn): Mark MIPS16_PIC_TEMP and
            MIPS_PROLOGUE_TEMP clobbered if MIPS16 and CALL_CLOBBERED_GP.

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[syq at gcc dot gnu.org]

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

YunQiang Su <syq at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #16 from YunQiang Su <syq at gcc dot gnu.org> ---
Fixed by https://gcc.gnu.org/g:915440eed21de367cb41857afb5273aff5bcb737

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[mschiffer@universe-factory.net]

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

--- Comment #17 from Matthias Schiffer <mschiffer@universe-factory.net> ---
I have now verified replacing Felix's patch with your new patch in the OpenWrt
toolchain (currently based on GCC 13.3) results in correct compilation, while a
GCC 13.3 without these patches applied exhibits the reported issue.

Thanks!

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[cvs-commit at gcc dot gnu.org]

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

--- Comment #18 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-14 branch has been updated by YunQiang Su <syq@gcc.gnu.org>:

https://gcc.gnu.org/g:201cfa725587d13867b4dc25955434ebe90aff7b

commit r14-10260-g201cfa725587d13867b4dc25955434ebe90aff7b
Author: YunQiang Su <syq@gcc.gnu.org>
Date:   Wed May 29 02:28:25 2024 +0800

    MIPS16: Mark $2/$3 as clobbered if GP is used

    PR Target/84790.
    The gp init sequence
            li      $2,%hi(_gp_disp)
            addiu   $3,$pc,%lo(_gp_disp)
            sll     $2,16
            addu    $2,$3
    is generated directly in `mips_output_function_prologue`, and does
    not appear in the RTL.

    So the IRA/IPA passes are not aware that $2/$3 have been clobbered,
    so they may be used for cross (local) function call.

    Let's mark $2/$3 clobber both:
      - Just after the UNSPEC_GP RTL of a function;
      - Just after a function call.

    Reported-by: Matthias Schiffer <mschiffer@universe-factory.net>
    Origin-Patch-by: Felix Fietkau <nbd@nbd.name>.

    gcc
            * config/mips/mips.cc(mips16_gp_pseudo_reg): Mark
            MIPS16_PIC_TEMP and MIPS_PROLOGUE_TEMP clobbered.
            (mips_emit_call_insn): Mark MIPS16_PIC_TEMP and
            MIPS_PROLOGUE_TEMP clobbered if MIPS16 and CALL_CLOBBERED_GP.

    (cherry picked from commit 915440eed21de367cb41857afb5273aff5bcb737)

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[cvs-commit at gcc dot gnu.org]

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

--- Comment #19 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by YunQiang Su <syq@gcc.gnu.org>:

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

commit r12-10480-ge26f16424f6279662efb210bc87c77148e956fed
Author: YunQiang Su <syq@gcc.gnu.org>
Date:   Wed May 29 02:28:25 2024 +0800

    MIPS16: Mark $2/$3 as clobbered if GP is used

    PR Target/84790.
    The gp init sequence
            li      $2,%hi(_gp_disp)
            addiu   $3,$pc,%lo(_gp_disp)
            sll     $2,16
            addu    $2,$3
    is generated directly in `mips_output_function_prologue`, and does
    not appear in the RTL.

    So the IRA/IPA passes are not aware that $2/$3 have been clobbered,
    so they may be used for cross (local) function call.

    Let's mark $2/$3 clobber both:
      - Just after the UNSPEC_GP RTL of a function;
      - Just after a function call.

    Reported-by: Matthias Schiffer <mschiffer@universe-factory.net>
    Origin-Patch-by: Felix Fietkau <nbd@nbd.name>.

    gcc
            * config/mips/mips.cc(mips16_gp_pseudo_reg): Mark
            MIPS16_PIC_TEMP and MIPS_PROLOGUE_TEMP clobbered.
            (mips_emit_call_insn): Mark MIPS16_PIC_TEMP and
            MIPS_PROLOGUE_TEMP clobbered if MIPS16 and CALL_CLOBBERED_GP.

    (cherry picked from commit 915440eed21de367cb41857afb5273aff5bcb737)

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[cvs-commit at gcc dot gnu.org]

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

--- Comment #20 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-13 branch has been updated by YunQiang Su <syq@gcc.gnu.org>:

https://gcc.gnu.org/g:3be8fa7b19d218ca5812d71801e3e83ee2260ea0

commit r13-8809-g3be8fa7b19d218ca5812d71801e3e83ee2260ea0
Author: YunQiang Su <syq@gcc.gnu.org>
Date:   Wed May 29 02:28:25 2024 +0800

    MIPS16: Mark $2/$3 as clobbered if GP is used

    PR Target/84790.
    The gp init sequence
            li      $2,%hi(_gp_disp)
            addiu   $3,$pc,%lo(_gp_disp)
            sll     $2,16
            addu    $2,$3
    is generated directly in `mips_output_function_prologue`, and does
    not appear in the RTL.

    So the IRA/IPA passes are not aware that $2/$3 have been clobbered,
    so they may be used for cross (local) function call.

    Let's mark $2/$3 clobber both:
      - Just after the UNSPEC_GP RTL of a function;
      - Just after a function call.

    Reported-by: Matthias Schiffer <mschiffer@universe-factory.net>
    Origin-Patch-by: Felix Fietkau <nbd@nbd.name>.

    gcc
            * config/mips/mips.cc(mips16_gp_pseudo_reg): Mark
            MIPS16_PIC_TEMP and MIPS_PROLOGUE_TEMP clobbered.
            (mips_emit_call_insn): Mark MIPS16_PIC_TEMP and
            MIPS_PROLOGUE_TEMP clobbered if MIPS16 and CALL_CLOBBERED_GP.

    (cherry picked from commit 915440eed21de367cb41857afb5273aff5bcb737)

[Bug target/84790] Miscompilation for MIPS16 with -fpic and -Os or -O2

[cvs-commit at gcc dot gnu.org]

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

--- Comment #21 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by YunQiang Su <syq@gcc.gnu.org>:

https://gcc.gnu.org/g:1bc4a777b21ae36b116e1842b7c482340ec929ef

commit r11-11457-g1bc4a777b21ae36b116e1842b7c482340ec929ef
Author: YunQiang Su <syq@gcc.gnu.org>
Date:   Wed May 29 02:28:25 2024 +0800

    MIPS16: Mark $2/$3 as clobbered if GP is used

    PR Target/84790.
    The gp init sequence
            li      $2,%hi(_gp_disp)
            addiu   $3,$pc,%lo(_gp_disp)
            sll     $2,16
            addu    $2,$3
    is generated directly in `mips_output_function_prologue`, and does
    not appear in the RTL.

    So the IRA/IPA passes are not aware that $2/$3 have been clobbered,
    so they may be used for cross (local) function call.

    Let's mark $2/$3 clobber both:
      - Just after the UNSPEC_GP RTL of a function;
      - Just after a function call.

    Reported-by: Matthias Schiffer <mschiffer@universe-factory.net>
    Origin-Patch-by: Felix Fietkau <nbd@nbd.name>.

    gcc
            * config/mips/mips.c(mips16_gp_pseudo_reg): Mark
            MIPS16_PIC_TEMP and MIPS_PROLOGUE_TEMP clobbered.
            (mips_emit_call_insn): Mark MIPS16_PIC_TEMP and
            MIPS_PROLOGUE_TEMP clobbered if MIPS16 and CALL_CLOBBERED_GP.

    (cherry picked from commit 915440eed21de367cb41857afb5273aff5bcb737)