[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[linkw at gcc dot gnu.org]

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

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
           Assignee|unassigned at gcc dot gnu.org      |linkw at gcc dot gnu.org
   Last reconfirmed|                            |2022-08-29

--- Comment #2 from Kewen Lin <linkw at gcc dot gnu.org> ---
Confirmed, I can reproduce it with cfarm machine gcc110, the issue is exactly
like what its comment describes. I have a patch.

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[cvs-commit at gcc dot gnu.org]

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

--- Comment #3 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Kewen Lin <linkw@gcc.gnu.org>:

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

commit r13-4894-gacc727cf02a1446dc00f8772f3f479fa3a508f8e
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Tue Dec 27 04:13:07 2022 -0600

    rs6000: Rework option -mpowerpc64 handling [PR106680]

    PR106680 shows that -m32 -mpowerpc64 is different from
    -mpowerpc64 -m32, this is determined by the way how we
    handle option powerpc64 in rs6000_handle_option.

    Segher pointed out this difference should be taken as
    a bug and we should ensure that option powerpc64 is
    independent of -m32/-m64.  So this patch removes the
    handlings in rs6000_handle_option and add some necessary
    supports in rs6000_option_override_internal instead.

    With this patch, if users specify -m{no-,}powerpc64, the
    specified value is honoured, otherwise, for 64bit it
    always enables OPTION_MASK_POWERPC64; while for 32bit
    and TARGET_POWERPC64 and OS_MISSING_POWERPC64, it disables
    OPTION_MASK_POWERPC64.

    btw, following Segher's suggestion, I did some tries to warn
    when OPTION_MASK_POWERPC64 is set for OS_MISSING_POWERPC64.
    If warn for the case that powerpc64 is specified explicitly,
    there are some TCs using -m32 -mpowerpc64 on ppc64-linux,
    they need some updates, meanwhile the artificial run
    with "--target_board=unix'{-m32/-mpowerpc64}'" will have
    noisy warnings on ppc64-linux.  If warn for the case that
    it's specified implicitly, they can just be initialized by
    TARGET_DEFAULT (like -m32 on ppc64-linux) or set from the
    given cpu mask, we have to special case them and not to warn.
    As Segher's latest comment, I decide not to warn them and
    keep it consistent with before.

    Bootstrapped and regress-tested on:
      - powerpc64-linux-gnu P7 and P8 {-m64,-m32}
      - powerpc64le-linux-gnu P9 and P10
      - powerpc-ibm-aix7.2.0.0 {-maix64,-maix32}
      - powerpc-darwin9 (with Iain's help)

            PR target/106680

    gcc/ChangeLog:

            * common/config/rs6000/rs6000-common.cc (rs6000_handle_option):
Remove
            the adjustment for option powerpc64 in -m64 handling, and remove
the
            whole -m32 handling.
            * config/rs6000/rs6000.cc (rs6000_option_override_internal): When
no
            explicit powerpc64 option is provided, enable it for -m64.  For 32
bit
            and OS_MISSING_POWERPC64, disable powerpc64 if it's enabled but not
            specified explicitly.

    gcc/testsuite/ChangeLog:

            * gcc.target/powerpc/pr106680-1.c: New test.
            * gcc.target/powerpc/pr106680-2.c: New test.
            * gcc.target/powerpc/pr106680-3.c: New test.
            * gcc.target/powerpc/pr106680-4.c: New test.

    2022-12-27  Kewen Lin  <linkw@linux.ibm.com>
                Iain Sandoe  <iain@sandoe.co.uk>

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[linkw at gcc dot gnu.org]

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

Kewen Lin <linkw at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
   Target Milestone|---                         |13.0
             Status|ASSIGNED                    |RESOLVED

--- Comment #4 from Kewen Lin <linkw at gcc dot gnu.org> ---
Should be fixed on trunk.

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[sebastian.huber@embedded-brains.de]

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

Sebastian Huber <sebastian.huber@embedded-brains.de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sebastian.huber@embedded-br
                   |                            |ains.de

--- Comment #5 from Sebastian Huber <sebastian.huber@embedded-brains.de> ---
Created attachment 57174
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57174&action=edit
Proprocessed test file.

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[sebastian.huber@embedded-brains.de]

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

--- Comment #6 from Sebastian Huber <sebastian.huber@embedded-brains.de> ---
It seems that the change

commit acc727cf02a1446dc00f8772f3f479fa3a508f8e
Author: Kewen Lin <linkw@linux.ibm.com>
Date:   Tue Dec 27 04:13:07 2022 -0600

    rs6000: Rework option -mpowerpc64 handling [PR106680]

causes a regression for -mcpu=e6500 -m32, for example:

gcc -fpreprocessed -O2 -S -mcpu=e6500 -m32 -S imfs_add_node.c.67.s
imfs_add_node.c.67.i

diff -u imfs_add_node.c.67.s.good.e2acff49fb2962b921bf8b73984b89878b61492c
imfs_add_node.c.67.s.bad.acc727cf02a1446dc00f8772f3f479fa3a508f8e
--- imfs_add_node.c.67.s.good.e2acff49fb2962b921bf8b73984b89878b61492c 
2024-01-20 12:15:15.143182571 +0100
+++ imfs_add_node.c.67.s.bad.acc727cf02a1446dc00f8772f3f479fa3a508f8e  
2024-01-20 12:11:46.804204927 +0100
@@ -52,8 +52,8 @@
        bne- 0,.L4
 .L2:
        mr 4,29
-       addi 3,1,8
        li 5,24
+       addi 3,1,8
        bl rtems_filesystem_eval_path_start
        lis 9,IMFS_node_clone@ha
        lwz 10,20(3)
@@ -63,12 +63,12 @@
        cmpw 0,10,9
        beq- 0,.L24
        li 4,134
-       addi 3,1,8
+       li 3,0
        bl rtems_filesystem_eval_path_error
 .L9:
        li 31,-1
 .L10:
-       addi 3,1,8
+       li 3,0
        bl rtems_filesystem_eval_path_cleanup
 .L1:
        lwz 0,116(1)
@@ -93,7 +93,7 @@
        lwz 9,12(31)
        li 8,96
        lhz 10,16(31)
-       addi 3,1,8
+       li 3,0
        stw 8,24(1)
        stw 9,8(1)
        stw 10,12(1)
@@ -105,7 +105,7 @@
        cmpwi 0,9,0
        beq- 0,.L9
        li 4,22
-       addi 3,1,8
+       li 3,0
        bl rtems_filesystem_eval_path_error
        b .L9
        .p2align 4,,15
@@ -129,12 +129,9 @@
        stw 9,0(10)
        stw 10,4(9)
        bl _Timecounter_Getbintime
-       lwz 10,64(1)
-       lwz 11,68(1)
-       stw 10,40(30)
-       stw 11,44(30)
-       stw 10,48(30)
-       stw 11,52(30)
+       ld 9,64(1)
+       std 9,40(30)
+       std 9,48(30)
        b .L10
        .cfi_endproc
 .LFE351:

For the call to rtems_filesystem_eval_path_cleanup() the register 3 should
point to a structure on the stack. Correct is:

-       addi 3,1,8

Wrong is:

+       li 3,0

It seems that for the -mcpu=e6500 the -m32 option has not the right effect and
some 64-bit instructions are generated, for example ld and std plus the wrong
function parameters.

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[linkw at gcc dot gnu.org]

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

--- Comment #7 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Sebastian Huber from comment #6)
> It seems that the change
> 
> commit acc727cf02a1446dc00f8772f3f479fa3a508f8e
> Author: Kewen Lin <linkw@linux.ibm.com>
> Date:   Tue Dec 27 04:13:07 2022 -0600
> 
>     rs6000: Rework option -mpowerpc64 handling [PR106680]
> 
> causes a regression for -mcpu=e6500 -m32, for example:
> 
> gcc -fpreprocessed -O2 -S -mcpu=e6500 -m32 -S imfs_add_node.c.67.s
> imfs_add_node.c.67.i
> 
> diff -u imfs_add_node.c.67.s.good.e2acff49fb2962b921bf8b73984b89878b61492c
> imfs_add_node.c.67.s.bad.acc727cf02a1446dc00f8772f3f479fa3a508f8e
> --- imfs_add_node.c.67.s.good.e2acff49fb2962b921bf8b73984b89878b61492c 
> 2024-01-20 12:15:15.143182571 +0100
> +++ imfs_add_node.c.67.s.bad.acc727cf02a1446dc00f8772f3f479fa3a508f8e  
> 2024-01-20 12:11:46.804204927 +0100
> @@ -52,8 +52,8 @@
>         bne- 0,.L4
>  .L2:
>         mr 4,29
> -       addi 3,1,8
>         li 5,24
> +       addi 3,1,8
>         bl rtems_filesystem_eval_path_start
>         lis 9,IMFS_node_clone@ha
>         lwz 10,20(3)
> @@ -63,12 +63,12 @@
>         cmpw 0,10,9
>         beq- 0,.L24
>         li 4,134
> -       addi 3,1,8
> +       li 3,0
>         bl rtems_filesystem_eval_path_error
>  .L9:
>         li 31,-1
>  .L10:
> -       addi 3,1,8
> +       li 3,0
>         bl rtems_filesystem_eval_path_cleanup
>  .L1:
>         lwz 0,116(1)
> @@ -93,7 +93,7 @@
>         lwz 9,12(31)
>         li 8,96
>         lhz 10,16(31)
> -       addi 3,1,8
> +       li 3,0
>         stw 8,24(1)
>         stw 9,8(1)
>         stw 10,12(1)
> @@ -105,7 +105,7 @@
>         cmpwi 0,9,0
>         beq- 0,.L9
>         li 4,22
> -       addi 3,1,8
> +       li 3,0
>         bl rtems_filesystem_eval_path_error
>         b .L9
>         .p2align 4,,15
> @@ -129,12 +129,9 @@
>         stw 9,0(10)
>         stw 10,4(9)
>         bl _Timecounter_Getbintime
> -       lwz 10,64(1)
> -       lwz 11,68(1)
> -       stw 10,40(30)
> -       stw 11,44(30)
> -       stw 10,48(30)
> -       stw 11,52(30)
> +       ld 9,64(1)
> +       std 9,40(30)
> +       std 9,48(30)
>         b .L10
>         .cfi_endproc
>  .LFE351:
> 
> For the call to rtems_filesystem_eval_path_cleanup() the register 3 should
> point to a structure on the stack. Correct is:
> 
> -       addi 3,1,8
> 
> Wrong is:
> 
> +       li 3,0
> 
> It seems that for the -mcpu=e6500 the -m32 option has not the right effect
> and some 64-bit instructions are generated, for example ld and std plus the

As the commit log, the previous behavior that -m32 also disables -mpowerpc64 is
wrong, -m{no,}powerpc64 should be independent of -m32/-m64.

> wrong function parameters.

I supposed that the behavior you wanted with -m32 is not to enable powerpc64
(since the previous behavior is -m32 can disable -mpowerpc64 as well), so I
think you can get the previous behavior if you specify one explicit
-mno-powerpc64 when adopting -m32.

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[sebastian.huber@embedded-brains.de]

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

--- Comment #8 from Sebastian Huber <sebastian.huber@embedded-brains.de> ---
Yes, it seems that -mcpu=e6500 -mno-powerpc64 yields the right code for the
attached test case (with or without the -m32).

I am now a bit confused what the purpose of the -m32 and -m64 options is.

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[linkw at gcc dot gnu.org]

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

--- Comment #9 from Kewen Lin <linkw at gcc dot gnu.org> ---
Note that now we only disable implicit powerpc64 for -m32 when the
OS_MISSING_POWERPC64 is set.

  /* Don't expect powerpc64 enabled on those OSes with OS_MISSING_POWERPC64,
     since they do not save and restore the high half of the GPRs correctly
     in all cases.  If the user explicitly specifies it, we won't interfere
     with the user's specification.  */
#ifdef OS_MISSING_POWERPC64
  if (OS_MISSING_POWERPC64
      && TARGET_32BIT
      && TARGET_POWERPC64
      && !(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64))
    rs6000_isa_flags &= ~OPTION_MASK_POWERPC64;
#endif

But rtems.h doesn't define OS_MISSING_POWERPC64

gcc/config/rs6000/linux.h:#define OS_MISSING_POWERPC64 1
gcc/config/rs6000/freebsd64.h:#define OS_MISSING_POWERPC64 !TARGET_64BIT
gcc/config/rs6000/aix.h:#define OS_MISSING_POWERPC64 1
gcc/config/rs6000/linux64.h:#define OS_MISSING_POWERPC64 !TARGET_64BIT

meanwhile cpu "e6500" has MASK_POWERPC64 set by default (it's 64bit core).

That's why you still have powerpc64 flag set when you specify -m32 on rtems.

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[sebastian.huber@embedded-brains.de]

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

--- Comment #10 from Sebastian Huber <sebastian.huber@embedded-brains.de> ---
(In reply to Kewen Lin from comment #9)
> Note that now we only disable implicit powerpc64 for -m32 when the
> OS_MISSING_POWERPC64 is set.
> 
>   /* Don't expect powerpc64 enabled on those OSes with OS_MISSING_POWERPC64,
>      since they do not save and restore the high half of the GPRs correctly
>      in all cases.  If the user explicitly specifies it, we won't interfere
>      with the user's specification.  */
> #ifdef OS_MISSING_POWERPC64
>   if (OS_MISSING_POWERPC64
>       && TARGET_32BIT
>       && TARGET_POWERPC64
>       && !(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64))
>     rs6000_isa_flags &= ~OPTION_MASK_POWERPC64;
> #endif
> 
> But rtems.h doesn't define OS_MISSING_POWERPC64

RTEMS supports the 64-bit PowerPC for the 64-bit multilibs.

> 
> gcc/config/rs6000/linux.h:#define OS_MISSING_POWERPC64 1
> gcc/config/rs6000/freebsd64.h:#define OS_MISSING_POWERPC64 !TARGET_64BIT
> gcc/config/rs6000/aix.h:#define OS_MISSING_POWERPC64 1
> gcc/config/rs6000/linux64.h:#define OS_MISSING_POWERPC64 !TARGET_64BIT
> 
> meanwhile cpu "e6500" has MASK_POWERPC64 set by default (it's 64bit core).
> 
> That's why you still have powerpc64 flag set when you specify -m32 on rtems.

For some applications, you don't need the 64-bit support on the e6500 machines.
So, we have 32-bit and 64-bit multilibs. This is just a performance
optimization for some applications.

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[linkw at gcc dot gnu.org]

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

--- Comment #11 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Sebastian Huber from comment #8)
> Yes, it seems that -mcpu=e6500 -mno-powerpc64 yields the right code for the
> attached test case (with or without the -m32).

The default is -m32 I guess? :)

> 
> I am now a bit confused what the purpose of the -m32 and -m64 options is.

For -m32/-m64, the manual says:

Generate code for 32-bit or 64-bit environments of Darwin and SVR4 targets
(including GNU/Linux). The 32-bit environment sets int, long and pointer to 32
bits and generates code that runs on any PowerPC variant. The 64-bit
environment sets int to 32 bits and long and pointer to 64 bits, and generates
code for PowerPC64, as for -mpowerpc64.

But it's possible to interact with option powerpc64, like cpu e6500 which by
default supports powerpc64 and if applied OS is able to support the necessary
context switches, we want -mpowerpc64 kept and it's able to generate more
efficient code (leveraging insns guarded with powerpc64 flag).

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[linkw at gcc dot gnu.org]

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

--- Comment #12 from Kewen Lin <linkw at gcc dot gnu.org> ---
(In reply to Sebastian Huber from comment #10)
> (In reply to Kewen Lin from comment #9)
> > Note that now we only disable implicit powerpc64 for -m32 when the
> > OS_MISSING_POWERPC64 is set.
> > 
> >   /* Don't expect powerpc64 enabled on those OSes with OS_MISSING_POWERPC64,
> >      since they do not save and restore the high half of the GPRs correctly
> >      in all cases.  If the user explicitly specifies it, we won't interfere
> >      with the user's specification.  */
> > #ifdef OS_MISSING_POWERPC64
> >   if (OS_MISSING_POWERPC64
> >       && TARGET_32BIT
> >       && TARGET_POWERPC64
> >       && !(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64))
> >     rs6000_isa_flags &= ~OPTION_MASK_POWERPC64;
> > #endif
> > 
> > But rtems.h doesn't define OS_MISSING_POWERPC64
> 
> RTEMS supports the 64-bit PowerPC for the 64-bit multilibs.
> 

64-bit kernel should support 64-bit PowerPC, but does 32-bit kernel support
saving and restoring 64-bit regs?

The current rtems.h is saying yes, if it's no, we should fix the rtems.h and
you won't need the explicit -mno-powerpc64 then.


btw, take the comments in freebsd64.h for example.

/* FreeBSD doesn't support saving and restoring 64-bit regs with a 32-bit
   kernel. This is supported when running on a 64-bit kernel with
   COMPAT_FREEBSD32, but tell GCC it isn't so that our 32-bit binaries
   are compatible. */
#define OS_MISSING_POWERPC64 !TARGET_64BIT

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[sebastian.huber@embedded-brains.de]

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

--- Comment #13 from Sebastian Huber <sebastian.huber@embedded-brains.de> ---
(In reply to Kewen Lin from comment #12)
> (In reply to Sebastian Huber from comment #10)
> > (In reply to Kewen Lin from comment #9)
> > > Note that now we only disable implicit powerpc64 for -m32 when the
> > > OS_MISSING_POWERPC64 is set.
> > > 
> > >   /* Don't expect powerpc64 enabled on those OSes with OS_MISSING_POWERPC64,
> > >      since they do not save and restore the high half of the GPRs correctly
> > >      in all cases.  If the user explicitly specifies it, we won't interfere
> > >      with the user's specification.  */
> > > #ifdef OS_MISSING_POWERPC64
> > >   if (OS_MISSING_POWERPC64
> > >       && TARGET_32BIT
> > >       && TARGET_POWERPC64
> > >       && !(rs6000_isa_flags_explicit & OPTION_MASK_POWERPC64))
> > >     rs6000_isa_flags &= ~OPTION_MASK_POWERPC64;
> > > #endif
> > > 
> > > But rtems.h doesn't define OS_MISSING_POWERPC64
> > 
> > RTEMS supports the 64-bit PowerPC for the 64-bit multilibs.
> > 
> 
> 64-bit kernel should support 64-bit PowerPC, but does 32-bit kernel support
> saving and restoring 64-bit regs?

For the 32-bit multilibs, we don't save/restore the full 64-bit registers.

> 
> The current rtems.h is saying yes, if it's no, we should fix the rtems.h and
> you won't need the explicit -mno-powerpc64 then.
> 
> 
> btw, take the comments in freebsd64.h for example.
> 
> /* FreeBSD doesn't support saving and restoring 64-bit regs with a 32-bit
>    kernel. This is supported when running on a 64-bit kernel with
>    COMPAT_FREEBSD32, but tell GCC it isn't so that our 32-bit binaries
>    are compatible. */
> #define OS_MISSING_POWERPC64 !TARGET_64BIT

Thanks for the hint, I will try out this setting.

[Bug testsuite/106680] Test gcc.target/powerpc/bswap64-4.c fails on 32-bit BE

[sebastian.huber@embedded-brains.de]

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

--- Comment #14 from Sebastian Huber <sebastian.huber@embedded-brains.de> ---
Thanks for your help, it seems that this patch fixes the issue for RTEMS:

diff --git a/gcc/config/rs6000/rtems.h b/gcc/config/rs6000/rtems.h
index 57a2325991b..b36e64fec77 100644
--- a/gcc/config/rs6000/rtems.h
+++ b/gcc/config/rs6000/rtems.h
@@ -36,6 +36,10 @@
 #endif
 #endif

+/* RTEMS configured for the 32-bit multilibs doesn't support saving and
+   restoring 64-bit regs.  */
+#define OS_MISSING_POWERPC64 !TARGET_64BIT
+
 /* Copy and paste from linux64.h and freebsd64.h */
 #undef TARGET_AIX
 #define        TARGET_AIX TARGET_64BIT