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

[cvs-commit at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #21 from GCC Commits  ---
The releases/gcc-11 branch has been updated by YunQiang Su :

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

commit r11-11457-g1bc4a777b21ae36b116e1842b7c482340ec929ef
Author: YunQiang Su 
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 
Origin-Patch-by: Felix Fietkau .

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)

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

[cvs-commit at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #20 from GCC Commits  ---
The releases/gcc-13 branch has been updated by YunQiang Su :

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

commit r13-8809-g3be8fa7b19d218ca5812d71801e3e83ee2260ea0
Author: YunQiang Su 
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 
Origin-Patch-by: Felix Fietkau .

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 via Gcc-bugs]

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

--- Comment #19 from GCC Commits  ---
The releases/gcc-12 branch has been updated by YunQiang Su :

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

commit r12-10480-ge26f16424f6279662efb210bc87c77148e956fed
Author: YunQiang Su 
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 
Origin-Patch-by: Felix Fietkau .

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 via Gcc-bugs]

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

--- Comment #18 from GCC Commits  ---
The releases/gcc-14 branch has been updated by YunQiang Su :

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

commit r14-10260-g201cfa725587d13867b4dc25955434ebe90aff7b
Author: YunQiang Su 
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 
Origin-Patch-by: Felix Fietkau .

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

[mschiffer--- via Gcc-bugs]

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

--- Comment #17 from Matthias Schiffer  ---
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

[syq at gcc dot gnu.org via Gcc-bugs]

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

YunQiang Su  changed:

   What|Removed |Added

 Status|ASSIGNED|RESOLVED
 Resolution|--- |FIXED

--- Comment #16 from YunQiang Su  ---
Fixed by https://gcc.gnu.org/g:915440eed21de367cb41857afb5273aff5bcb737

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

[cvs-commit at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #15 from GCC Commits  ---
The master branch has been updated by YunQiang Su :

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

commit r15-911-g915440eed21de367cb41857afb5273aff5bcb737
Author: YunQiang Su 
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 
Origin-Patch-by: Felix Fietkau .

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 via Gcc-bugs]

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

--- Comment #14 from YunQiang Su  ---
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

[mschiffer--- via Gcc-bugs]

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

--- Comment #13 from Matthias Schiffer  ---
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:

 :
   0:   e8a0jrc ra
   2:   6500nop

0004 :
   4:   f000 6a00   li  v0,0
4: R_MIPS16_HI16_gp_disp
   8:   f000 0b00   la  v1,8 
8: R_MIPS16_LO16_gp_disp
   c:   f400 3240   sll v0,16
  10:   e269adduv0,v1
  12:   64c4save32,ra
  14:   659amovegp,v0
  16:   d204sw  v0,16(sp)
  18:   675cmovev0,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:   ea40jalrv0
  24:   653amovet9,v0
  26:   6444restore 32,ra
  28:   e8a0jrc ra
  2a:   6500nop

002c :
  2c:   f000 6a00   li  v0,0
2c: R_MIPS16_HI16   _gp_disp
  30:   f000 0b00   la  v1,30 
30: R_MIPS16_LO16   _gp_disp
  34:   f400 3240   sll v0,16
  38:   e269adduv0,v1
  3a:   659amovegp,v0
  3c:   64e4save32,ra,s0
  3e:   671cmoves0,gp
  40:   d204sw  v0,16(sp)
  42:   f000 9840   lw  v0,0(s0)
42: R_MIPS16_CALL16 ext
  46:   ea40jalrv0
  48:   653amovet9,v0
  4a:   6762movev1,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:   e840jalrs0
  56:   6538movet9,s0
  58:   6464restore 32,ra,s0
  5a:   e820jr  ra
  5c:   6743movev0,v1
  5e:   6500nop
```

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 via Gcc-bugs]

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

--- Comment #12 from YunQiang Su  ---
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

[syq at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #11 from YunQiang Su  ---
(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

[mschiffer--- via Gcc-bugs]

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

--- Comment #10 from Matthias Schiffer  ---
(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 via Gcc-bugs]

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

--- Comment #9 from YunQiang Su  ---
(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

[syq at gcc dot gnu.org via Gcc-bugs]

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

--- Comment #8 from YunQiang Su  ---
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

[mschiffer--- via Gcc-bugs]

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

--- Comment #7 from Matthias Schiffer  ---
(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 via Gcc-bugs]

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

--- Comment #6 from YunQiang Su  ---
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

[syq at gcc dot gnu.org via Gcc-bugs]

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

YunQiang Su  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  ---
Assign it to me.

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

[ebotcazou at gcc dot gnu.org]

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

Eric Botcazou  changed:

   What|Removed |Added

 CC||clm at codesourcery dot com,
   ||ebotcazou at gcc dot gnu.org,
   ||matthew.fortune at imgtec dot 
com

--- Comment #4 from Eric Botcazou  ---
> I've hacked up a patch that seems to fix this issue, but I have no idea if
> the approach is correct.

This might slightly pessimize (maybe test mips_symbol_binds_local_p?) but only
a MIPS maintainer can give an informed opinion here, so CCing them.

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

[nbd at nbd dot name]

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

Felix Fietkau  changed:

   What|Removed |Added

 CC||nbd at nbd dot name

--- Comment #3 from Felix Fietkau  ---
Created attachment 43627
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=43627=edit
Proposed fix

I've hacked up a patch that seems to fix this issue, but I have no idea if the
approach is correct.

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

[mschif...@universe-factory.net]

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

--- Comment #2 from Matthias Schiffer  ---
The problem seems to be that the gp init sequence

li  $2,%hi(_gp_disp)
addiu   $3,$pc,%lo(_gp_disp)
sll $2,16
addu$2,$3

is generated very late and does not appear in the RTL in any way, so optimizing
passes are not aware of the $3 (and possibly $2?) clobber. I don't know enough
about GCC internals for further analysis.

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

[mschif...@universe-factory.net]

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

--- Comment #1 from Matthias Schiffer  ---
Issue still present in gcc version 8.0.1 20180310 (experimental) (GCC). Again,
output it identical to that of GCC 5 and 7.