diff mbox series

[v2] rs6000: Fix .machine cpu selection w/ altivec [PR97367]

Message ID 5f2b5d5e-a682-4084-b70e-89929f4cc6dc@bergner.org
State New
Headers show
Series [v2] rs6000: Fix .machine cpu selection w/ altivec [PR97367] | expand

Commit Message

Peter Bergner July 12, 2024, 9:48 p.m. UTC 
René's patch seems to have stalled, so here is an updated version of the
patch with the requested changes to his patch.

I'll note I have added an additional code change, which is to also emit a
".machine altivec" if Altivec is enabled.  The problem this fixes is for
cpus like the G5, which is basically a power4 plus an Altivec unit, its
".machine power4" doesn't enable the assembler to recognize Altivec insns.
That isn't a problem if you use gcc -mcpu=G5 to assemble the assembler file,
since gcc passes -maltivec to the assembler.  However, if you try to assemble
the assembler file with as by hand, you'll get "unrecognized opcode" errors.
I did not do the same for VSX, since all ".machine <cpu>" for cpus that
support VSX already enable VSX insn recognition, so it's not needed.


rs6000: Fix .machine cpu selection w/ altivec [PR97367]

There are various non-IBM CPUs with altivec, so we cannot use that
flag to determine which .machine cpu to use, so ignore it.
Emit an additional ".machine altivec" if Altivec is enabled so
that the assembler doesn't require an explicit -maltivec option
to assemble any Altivec instructions for those targets where
the ".machine cpu" is insufficient to enable Altivec.  For example,
-mcpu=G5 emits a ".machine power4".

This passed bootstrap and regtesting on powrpc64-linux (running the testsuite
in both 32-bit and 64-bit modes) with no regressions.

Ok for trunk and the release branches after some trunk burn-in time?

Peter


2024-07-12  René Rebe  <rene@exactcode.de>
	    Peter Bergner  <bergner@linux.ibm.com>

gcc/
	PR target/97367
	* config/rs6000/rs6000.c (rs6000_machine_from_flags): Do not consider
	OPTION_MASK_ALTIVEC.
	(emit_asm_machine): For Altivec compiles, emit a ".machine altivec".

gcc/testsuite/
	PR target/97367
	* gcc.target/powerpc/pr97367.c: New test.

Signed-of-by: René Rebe <rene@exactcode.de>
---
 gcc/config/rs6000/rs6000.cc                |  5 ++++-
 gcc/testsuite/gcc.target/powerpc/pr97367.c | 13 +++++++++++++
 2 files changed, 17 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr97367.c

Comments

René Rebe July 15, 2024, 10:25 a.m. UTC | #1 
Hi Peter,

> On Jul 12, 2024, at 23:48, Peter Bergner <pshop@bergner.org> wrote:
> 
> René's patch seems to have stalled, so here is an updated version of the
> patch with the requested changes to his patch.

Yes, sorry. Our Linux distribution grew so much in popularity the last
month that I had hard weeks keeping up with all the new developments.

Your changes looks good to me, too. The question is do the maintainers
agree and wether they like this in one or two commits.

Signed-of-by: René Rebe <rene@exactcode.de>

> I'll note I have added an additional code change, which is to also emit a
> ".machine altivec" if Altivec is enabled.  The problem this fixes is for
> cpus like the G5, which is basically a power4 plus an Altivec unit, its
> ".machine power4" doesn't enable the assembler to recognize Altivec insns.
> That isn't a problem if you use gcc -mcpu=G5 to assemble the assembler file,
> since gcc passes -maltivec to the assembler.  However, if you try to assemble
> the assembler file with as by hand, you'll get "unrecognized opcode" errors.
> I did not do the same for VSX, since all ".machine <cpu>" for cpus that
> support VSX already enable VSX insn recognition, so it's not needed.
> 
> 
> rs6000: Fix .machine cpu selection w/ altivec [PR97367]
> 
> There are various non-IBM CPUs with altivec, so we cannot use that
> flag to determine which .machine cpu to use, so ignore it.
> Emit an additional ".machine altivec" if Altivec is enabled so
> that the assembler doesn't require an explicit -maltivec option
> to assemble any Altivec instructions for those targets where
> the ".machine cpu" is insufficient to enable Altivec.  For example,
> -mcpu=G5 emits a ".machine power4".
> 
> This passed bootstrap and regtesting on powrpc64-linux (running the testsuite
> in both 32-bit and 64-bit modes) with no regressions.
> 
> Ok for trunk and the release branches after some trunk burn-in time?
> 
> Peter
> 
> 
> 2024-07-12  René Rebe  <rene@exactcode.de>
> 	    Peter Bergner  <bergner@linux.ibm.com>
> 
> gcc/
> 	PR target/97367
> 	* config/rs6000/rs6000.c (rs6000_machine_from_flags): Do not consider
> 	OPTION_MASK_ALTIVEC.
> 	(emit_asm_machine): For Altivec compiles, emit a ".machine altivec".
> 
> gcc/testsuite/
> 	PR target/97367
> 	* gcc.target/powerpc/pr97367.c: New test.
> 
> Signed-of-by: René Rebe <rene@exactcode.de>
> ---
> gcc/config/rs6000/rs6000.cc                |  5 ++++-
> gcc/testsuite/gcc.target/powerpc/pr97367.c | 13 +++++++++++++
> 2 files changed, 17 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr97367.c
> 
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 2cbea6ea2d7..2cb8f35739b 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -5888,7 +5888,8 @@ rs6000_machine_from_flags (void)
>   HOST_WIDE_INT flags = rs6000_isa_flags;
> 
>   /* Disable the flags that should never influence the .machine selection.  */
> -  flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ISEL);
> +  flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ISEL
> +	     | OPTION_MASK_ALTIVEC);
> 
>   if ((flags & (ISA_3_1_MASKS_SERVER & ~ISA_3_0_MASKS_SERVER)) != 0)
>     return "power10";
> @@ -5913,6 +5914,8 @@ void
> emit_asm_machine (void)
> {
>   fprintf (asm_out_file, "\t.machine %s\n", rs6000_machine);
> +  if (TARGET_ALTIVEC)
> +    fprintf (asm_out_file, "\t.machine altivec\n");
> }
> #endif
> 
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr97367.c b/gcc/testsuite/gcc.target/powerpc/pr97367.c
> new file mode 100644
> index 00000000000..f9118dbcdec
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr97367.c
> @@ -0,0 +1,13 @@
> +/* PR target/97367 */
> +/* { dg-options "-mdejagnu-cpu=G5" } */
> +
> +/* Verify we emit a ".machine power4" and ".machine altivec" rather
> +   than a ".machine power7".  */
> +
> +int dummy (void)
> +{
> +  return 0;
> +}
> +
> +/* { dg-final { scan-assembler {\.\mmachine power4\M} } } */
> +/* { dg-final { scan-assembler {\.\mmachine altivec\M} } } */
> -- 
> 2.45.2
>
Kewen.Lin July 18, 2024, 9:14 a.m. UTC | #2 
Hi Peter,

on 2024/7/13 05:48, Peter Bergner wrote:
> René's patch seems to have stalled, so here is an updated version of the
> patch with the requested changes to his patch.
> 
> I'll note I have added an additional code change, which is to also emit a
> ".machine altivec" if Altivec is enabled.  The problem this fixes is for
> cpus like the G5, which is basically a power4 plus an Altivec unit, its
> ".machine power4" doesn't enable the assembler to recognize Altivec insns.
> That isn't a problem if you use gcc -mcpu=G5 to assemble the assembler file,
> since gcc passes -maltivec to the assembler.  However, if you try to assemble
> the assembler file with as by hand, you'll get "unrecognized opcode" errors.
> I did not do the same for VSX, since all ".machine <cpu>" for cpus that
> support VSX already enable VSX insn recognition, so it's not needed.

Sounds great, thanks for improving it.

> 
> 
> rs6000: Fix .machine cpu selection w/ altivec [PR97367]
> 
> There are various non-IBM CPUs with altivec, so we cannot use that
> flag to determine which .machine cpu to use, so ignore it.
> Emit an additional ".machine altivec" if Altivec is enabled so
> that the assembler doesn't require an explicit -maltivec option
> to assemble any Altivec instructions for those targets where
> the ".machine cpu" is insufficient to enable Altivec.  For example,
> -mcpu=G5 emits a ".machine power4".
> 
> This passed bootstrap and regtesting on powrpc64-linux (running the testsuite
> in both 32-bit and 64-bit modes) with no regressions.
> 
> Ok for trunk and the release branches after some trunk burn-in time?

OK for all with/without the below minor nit tweaked.

> 
> Peter
> 
> 
> 2024-07-12  René Rebe  <rene@exactcode.de>
> 	    Peter Bergner  <bergner@linux.ibm.com>
> 
> gcc/
> 	PR target/97367
> 	* config/rs6000/rs6000.c (rs6000_machine_from_flags): Do not consider
> 	OPTION_MASK_ALTIVEC.
> 	(emit_asm_machine): For Altivec compiles, emit a ".machine altivec".
> 
> gcc/testsuite/
> 	PR target/97367
> 	* gcc.target/powerpc/pr97367.c: New test.
> 
> Signed-of-by: René Rebe <rene@exactcode.de>
> ---
>  gcc/config/rs6000/rs6000.cc                |  5 ++++-
>  gcc/testsuite/gcc.target/powerpc/pr97367.c | 13 +++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/pr97367.c
> 
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 2cbea6ea2d7..2cb8f35739b 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -5888,7 +5888,8 @@ rs6000_machine_from_flags (void)
>    HOST_WIDE_INT flags = rs6000_isa_flags;
>  
>    /* Disable the flags that should never influence the .machine selection.  */
> -  flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ISEL);
> +  flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ISEL
> +	     | OPTION_MASK_ALTIVEC);
>  
>    if ((flags & (ISA_3_1_MASKS_SERVER & ~ISA_3_0_MASKS_SERVER)) != 0)
>      return "power10";
> @@ -5913,6 +5914,8 @@ void
>  emit_asm_machine (void)
>  {
>    fprintf (asm_out_file, "\t.machine %s\n", rs6000_machine);
> +  if (TARGET_ALTIVEC)
> +    fprintf (asm_out_file, "\t.machine altivec\n");
>  }
>  #endif
>  
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr97367.c b/gcc/testsuite/gcc.target/powerpc/pr97367.c
> new file mode 100644
> index 00000000000..f9118dbcdec
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr97367.c
> @@ -0,0 +1,13 @@
> +/* PR target/97367 */
> +/* { dg-options "-mdejagnu-cpu=G5" } */
> +
> +/* Verify we emit a ".machine power4" and ".machine altivec" rather
> +   than a ".machine power7".  */
> +
> +int dummy (void)
> +{
> +  return 0;
> +}
> +
> +/* { dg-final { scan-assembler {\.\mmachine power4\M} } } */
> +/* { dg-final { scan-assembler {\.\mmachine altivec\M} } } */

Nit: Both \m looks useless and can be removed.

BR,
Kewen
Peter Bergner July 18, 2024, 2:14 p.m. UTC | #3 
On 7/18/24 4:14 AM, Kewen.Lin wrote:
>> +/* { dg-final { scan-assembler {\.\mmachine power4\M} } } */
>> +/* { dg-final { scan-assembler {\.\mmachine altivec\M} } } */
> 
> Nit: Both \m looks useless and can be removed.

Fine with me.  Is that because the \. acts like a \m?



>> Ok for trunk and the release branches after some trunk burn-in time?
> 
> OK for all with/without the below minor nit tweaked.

Great, thanks!


Peter
Peter Bergner July 18, 2024, 6:17 p.m. UTC | #4 
On 7/18/24 9:14 AM, Peter Bergner wrote:
> On 7/18/24 4:14 AM, Kewen.Lin wrote:
>>> +/* { dg-final { scan-assembler {\.\mmachine power4\M} } } */
>>> +/* { dg-final { scan-assembler {\.\mmachine altivec\M} } } */
>>
>> Nit: Both \m looks useless and can be removed.
> 
> Fine with me.  Is that because the \. acts like a \m?
> 
> 
> 
>>> Ok for trunk and the release branches after some trunk burn-in time?
>>
>> OK for all with/without the below minor nit tweaked.

Ok, I pushed the commit with the tweak to trunk and will let it burn-in
there for a bit before backporting to the release branches.

Thanks René and Kewen!

Peter
Kewen.Lin July 19, 2024, 1:25 a.m. UTC | #5 
on 2024/7/18 22:14, Peter Bergner wrote:
> On 7/18/24 4:14 AM, Kewen.Lin wrote:
>>> +/* { dg-final { scan-assembler {\.\mmachine power4\M} } } */
>>> +/* { dg-final { scan-assembler {\.\mmachine altivec\M} } } */
>>
>> Nit: Both \m looks useless and can be removed.
> 
> Fine with me.  Is that because the \. acts like a \m?

Yes, \m is to make sure "machine" is starting at word boundary excluding
some unexpected matching on words like "timemachine", but the preceding
dot (\.) already guarantees that.

BR,
Kewen

> 
> 
> 
>>> Ok for trunk and the release branches after some trunk burn-in time?
>>
>> OK for all with/without the below minor nit tweaked.
> 
> Great, thanks!
> 
> 
> Peter
> 
>
diff mbox series

Patch

diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index 2cbea6ea2d7..2cb8f35739b 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -5888,7 +5888,8 @@  rs6000_machine_from_flags (void)
   HOST_WIDE_INT flags = rs6000_isa_flags;
 
   /* Disable the flags that should never influence the .machine selection.  */
-  flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ISEL);
+  flags &= ~(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_PPC_GPOPT | OPTION_MASK_ISEL
+	     | OPTION_MASK_ALTIVEC);
 
   if ((flags & (ISA_3_1_MASKS_SERVER & ~ISA_3_0_MASKS_SERVER)) != 0)
     return "power10";
@@ -5913,6 +5914,8 @@  void
 emit_asm_machine (void)
 {
   fprintf (asm_out_file, "\t.machine %s\n", rs6000_machine);
+  if (TARGET_ALTIVEC)
+    fprintf (asm_out_file, "\t.machine altivec\n");
 }
 #endif
 
diff --git a/gcc/testsuite/gcc.target/powerpc/pr97367.c b/gcc/testsuite/gcc.target/powerpc/pr97367.c
new file mode 100644
index 00000000000..f9118dbcdec
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr97367.c
@@ -0,0 +1,13 @@ 
+/* PR target/97367 */
+/* { dg-options "-mdejagnu-cpu=G5" } */
+
+/* Verify we emit a ".machine power4" and ".machine altivec" rather
+   than a ".machine power7".  */
+
+int dummy (void)
+{
+  return 0;
+}
+
+/* { dg-final { scan-assembler {\.\mmachine power4\M} } } */
+/* { dg-final { scan-assembler {\.\mmachine altivec\M} } } */