Hi Mike,
on 2024/1/12 01:29, Michael Meissner wrote:
> This is version 2 of the patch. The only difference is I made the test case
> simpler to read.
>
> In looking at support for load vector pair and store vector pair for the
> PowerPC in GCC, I noticed that we were missing a print_operand output modifier
> if you are dealing with vector pairs to print the 2nd register in the vector
> pair.
>
> If the instruction inside of the asm used the Altivec encoding, then we could
> use the %L<n> modifier:
>
> __vector_pair *p, *q, *r;
> // ...
> __asm__ ("vaddudm %0,%1,%2\n\tvaddudm %L0,%L1,%L2"
> : "=v" (*p)
> : "v" (*q), "v" (*r));
>
> Likewise if we know the value to be in a tradiational FPR register, %L<n> will
> work for instructions that use the VSX encoding:
>
> __vector_pair *p, *q, *r;
> // ...
> __asm__ ("xvadddp %x0,%x1,%x2\n\txvadddp %L0,%L1,%L2"
> : "=f" (*p)
> : "f" (*q), "f" (*r));
>
> But if have a value that is in a traditional Altivec register, and the
> instruction uses the VSX encoding, %L<n> will a value between 0 and 31, when
> it
> should give a value between 32 and 63.
>
> This patch adds %S<n> that acts like %x<n>, except that it adds 1 to the
> register number.
>
> I have tested this on power10 and power9 little endian systems and on a power9
> big endian system. There were no regressions in the patch. Can I apply it to
> the trunk?
>
> It would be nice if I could apply it to the open branches. Can I backport it
> after a burn-in period?
>
> 2024-01-10 Michael Meissner <meiss...@linux.ibm.com>
>
> gcc/
>
> PR target/112886
> * config/rs6000/rs6000.cc (print_operand): Add %S<n> output modifier.
> * doc/md.texi (Modifiers): Mention %S can be used like %x.
>
> gcc/testsuite/
>
> PR target/112886
> * /gcc.target/powerpc/pr112886.c: New test.
> ---
> gcc/config/rs6000/rs6000.cc | 10 ++++---
> gcc/doc/md.texi | 5 ++--
> gcc/testsuite/gcc.target/powerpc/pr112886.c | 29 +++++++++++++++++++++
> 3 files changed, 39 insertions(+), 5 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr112886.c
>
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index 872df3140e3..6353a7ccfb2 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -14504,13 +14504,17 @@ print_operand (FILE *file, rtx x, int code)
> print_operand (file, x, 0);
> return;
>
> + case 'S':
> case 'x':
> - /* X is a FPR or Altivec register used in a VSX context. */
> + /* X is a FPR or Altivec register used in a VSX context. %x<n> prints
> + the VSX register number, %S<n> prints the 2nd register number for
> + vector pair, decimal 128-bit floating and IBM 128-bit binary floating
> + values. */
> if (!REG_P (x) || !VSX_REGNO_P (REGNO (x)))
> - output_operand_lossage ("invalid %%x value");
> + output_operand_lossage ("invalid %%%c value", (code == 'S' ? 'S' :
> 'x'));
Nit: Seems simpler with
output_operand_lossage ("invalid %%%c value", (char) code);
> else
> {
> - int reg = REGNO (x);
> + int reg = REGNO (x) + (code == 'S' ? 1 : 0);
> int vsx_reg = (FP_REGNO_P (reg)
> ? reg - 32
> : reg - FIRST_ALTIVEC_REGNO + 32);
> diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
> index 47a87d6ceec..53ec957cb23 100644
> --- a/gcc/doc/md.texi
> +++ b/gcc/doc/md.texi
> @@ -3386,8 +3386,9 @@ A VSX register (VSR), @code{vs0}@dots{}@code{vs63}.
> This is either an
> FPR (@code{vs0}@dots{}@code{vs31} are @code{f0}@dots{}@code{f31}) or a VR
> (@code{vs32}@dots{}@code{vs63} are @code{v0}@dots{}@code{v31}).
>
> -When using @code{wa}, you should use the @code{%x} output modifier, so that
> -the correct register number is printed. For example:
> +When using @code{wa}, you should use either the @code{%x} or @code{%S}
> +output modifier, so that the correct register number is printed. For
> +example:
As I questioned in [1], "It seems there is no Power specific documentation on
operand modifiers like this %L?", even if we document this new "%S" as above,
users can still be confused.
Does it sound better with something like " ... @code{%x} or @code{%S} (for the
next consecutive register number in the context like vector pair etc.) ... "?
[1] https://gcc.gnu.org/pipermail/gcc-patches/2024-January/642414.html
>
> @smallexample
> asm ("xvadddp %x0,%x1,%x2"
And we can also extend this by adding one more example like your associated test
case (hope it can make it more clear).
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr112886.c
> b/gcc/testsuite/gcc.target/powerpc/pr112886.c
> new file mode 100644
> index 00000000000..4e59dcda6ea
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr112886.c
> @@ -0,0 +1,29 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target power10_ok } */
I think this needs one more:
/* { dg-require-effective-target powerpc_vsx_ok } */
> +/* { dg-options "-mdejagnu-cpu=power10 -O2" } */
... and
/* { dg-options "-mdejagnu-cpu=power10 -O2 -mvsx" } */
, otherwise with explicit -mno-vsx, this test case would fail.
The other looks good to me, thanks!
BR,
Kewen
> +
> +/* PR target/112886: Test that print_operand %S<n> gives the correct register
> + number for VSX registers (i.e. if the register is an Altivec register, the
> + register number is 32..63 instead of 0..31. */
> +
> +void
> +test (__vector_pair *ptr1, __vector_pair *ptr2, __vector_pair *ptr3)
> +{
> + register __vector_pair p asm ("vs10");
> + register __vector_pair q asm ("vs42");
> + register __vector_pair r asm ("vs44");
> +
> + q = *ptr2;
> + r = *ptr3;
> +
> + __asm__ ("xvadddp %x0,%x1,%x2\n\txvadddp %S0,%S1,%S2"
> + : "=wa" (p)
> + : "wa" (q), "wa" (r));
> +
> + *ptr1 = p;
> +}
> +
> +/* { dg-final { scan-assembler-times {\mxvadddp 10,42,44\M} 1 } } */
> +/* { dg-final { scan-assembler-times {\mxvadddp 11,43,45\M} 1 } } */
> +/* { dg-final { scan-assembler-times {\mlxvpx?\M} 2 } } */
> +/* { dg-final { scan-assembler-times {\mstxvpx?\M} 1 } } */
