Hi Carl,
on 2023/6/2 04:01, Carl Love wrote:
> Kewen, Segher, Peter:
>
> The following patch is a redo of the previous "rs6000: Fix
> __builtin_vec_xst_trunc definition" patch.
>
> This patch fixes the argument in the two builtin definitions
> __builtin_altivec_tr_stxvrwx and __builtin_altivec_tr_stxvrhx. It also
> adds with a testcase to validate the related builtins which have the
> third argument of char *, short *, int * and long long *.
>
> I have tested the patch on Power 10 with no regressions.
>
> Please let me know if this patch is acceptable for mainline.
Thanks for catching and fixing this.
OK for trunk with or without some nits below in test case fixed
(as it's just a test case :)).
>
> Carl
>
> ------------------------------------------------------------
> rs6000: Fix arguments for __builtin_altivec_tr_stxvrwx,
> __builtin_altivec_tr_stxvrhx
>
> The third argument for __builtin_altivec_tr_stxvrhx should be short *
> not int *. Similarly, the third argument for __builtin_altivec_tr_stxvrwx
> should be int * not short *. This patch fixes the arguments in the two
> builtins.
>
> A runnable test case is added to test the __builtin_altivec_tr_stxvrbx,
> __builtin_altivec_tr_stxvrhx, __builtin_altivec_tr_stxvrwx and
> __builtin_altivec_tr_stxvrdx builtins.
>
> gcc/
> * config/rs6000/rs6000-builtins.def (__builtin_altivec_tr_stxvrhx,
> __builtin_altivec_tr_stxvrwx): Fix type of third argument.
>
> gcc/testsuite/
> * gcc.target/powerpc/builtin_altivec_tr_stxvr_runnable.c: New test
> for __builtin_altivec_tr_stxvrbx, __builtin_altivec_tr_stxvrhx,
> __builtin_altivec_tr_stxvrwx, __builtin_altivec_tr_stxvrdx.
> ---
> gcc/config/rs6000/rs6000-builtins.def | 4 +-
> .../builtin_altivec_tr_stxvr_runnable.c | 107 ++++++++++++++++++
> 2 files changed, 109 insertions(+), 2 deletions(-)
> create mode 100644
> gcc/testsuite/gcc.target/powerpc/builtin_altivec_tr_stxvr_runnable.c
>
> diff --git a/gcc/config/rs6000/rs6000-builtins.def
> b/gcc/config/rs6000/rs6000-builtins.def
> index 638d0bc72ca..d7839f2e06b 100644
> --- a/gcc/config/rs6000/rs6000-builtins.def
> +++ b/gcc/config/rs6000/rs6000-builtins.def
> @@ -3161,10 +3161,10 @@
> void __builtin_altivec_tr_stxvrbx (vsq, signed long, signed char *);
> TR_STXVRBX vsx_stxvrbx {stvec}
>
> - void __builtin_altivec_tr_stxvrhx (vsq, signed long, signed int *);
> + void __builtin_altivec_tr_stxvrhx (vsq, signed long, signed short *);
> TR_STXVRHX vsx_stxvrhx {stvec}
>
> - void __builtin_altivec_tr_stxvrwx (vsq, signed long, signed short *);
> + void __builtin_altivec_tr_stxvrwx (vsq, signed long, signed int *);
> TR_STXVRWX vsx_stxvrwx {stvec}
>
> void __builtin_altivec_tr_stxvrdx (vsq, signed long, signed long long *);
> diff --git
> a/gcc/testsuite/gcc.target/powerpc/builtin_altivec_tr_stxvr_runnable.c
> b/gcc/testsuite/gcc.target/powerpc/builtin_altivec_tr_stxvr_runnable.c
> new file mode 100644
> index 00000000000..46014d83535
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/builtin_altivec_tr_stxvr_runnable.c
> @@ -0,0 +1,107 @@
> +/* Test of __builtin_vec_xst_trunc */
> +
> +/* { dg-do run { target power10_hw } } */
> +/* { dg-require-effective-target int128 } */
> +/* { dg-options "-mdejagnu-cpu=power10 -save-temps" } */
> +
> +#include <altivec.h>
> +#include <stdio.h>
> +#include <inttypes.h>
> +#include <string.h>
> +#include <stdlib.h>
> +
> +#define DEBUG 0
> +
> +vector signed __int128 store_data =
> + { (__int128) 0x8ACE000000000000 << 64 | (__int128) 0xfedcba9876543217ULL};
> +
> +union conv_t {
> + vector signed __int128 vsi128;
> + unsigned long long ull[2];
> +} conv;
> +
> +void abort (void);
> +
> +
> +int
> +main () {
> + int i;
> + signed long sl;
> + signed char sc, expected_sc;
> + signed short ss, expected_ss;
> + signed int si, expected_si;
> + signed long long int sll, expected_sll;
> + signed char *psc;
> + signed short *pss;
> + signed int *psi;
> + signed long long int *psll;
> +
> +#if DEBUG
> + val.vsi128 = store_data;
> + printf("Data to store [%d] = 0x%llx %llx\n", i, val.ull[1], val.ull[0]);
odd indent.
> +#endif
> +
> + psc = ≻
> + pss = &ss;
> + psi = &si;
> + psll = &sll;
> +
> + sl = 1;
> + sc =0xA1;
one more space after "=".
> + expected_sc = 0xA1;
> + __builtin_altivec_tr_stxvrbx (store_data, sl, psc);
> +
> + if (expected_sc != sc & 0xFF)
> +#if DEBUG
> + printf(" ERROR: Signed char = 0x%x doesn't match expected value 0x%x\n",
> + sc & 0xFF, expected_sc);
> +#else
> + abort();
> +#endif
> +
> + sl = 1;
redundant, and easy to cause misunderstanding that sl can get changed.
> + ss = 0x52;
> + expected_ss = 0x1752;
> + __builtin_altivec_tr_stxvrhx (store_data, sl, pss);
> +
> + if (expected_ss != ss & 0xFFFF)
> +#if DEBUG
> + printf(" ERROR: Signed short = 0x%x doesn't match expected value 0x%x\n",
> + ss, expected_ss) & 0xFFFF;
> +#else
> + abort();
> +#endif
> +
> + sl = 1;
same as above.
> + si = 0x21;
> + expected_si = 0x54321721;
> + __builtin_altivec_tr_stxvrwx (store_data, sl, psi);
odd indent.
> +
> + if (expected_si != si)
> +#if DEBUG
> + printf(" ERROR: Signed int = 0x%x doesn't match expected value 0x%x\n",
> + si, expected_si);
> +#else
> + abort();
> +#endif
> +
> + sl = 1;
same as above.
> + sll = 0x12FFULL;
> + expected_sll = 0xdcba9876543217FF;
> + __builtin_altivec_tr_stxvrdx (store_data, sl, psll);
Ditto.
BR,
Kewen
> +
> + if (expected_sll != sll)
> +#if DEBUG
> + printf(" ERROR: Signed long long int = 0x%llx doesn't match expected
> value 0x%llx\n",
> + sll, expected_sll);
> +#else
> + abort();
> +#endif
> +
> + return 0;
> +}
> +
> +/* { dg-final { scan-assembler-times {\mstxvrbx\M} 1 } } */
> +/* { dg-final { scan-assembler-times {\mstxvrhx\M} 1 } } */
> +/* { dg-final { scan-assembler-times {\mstxvrwx\M} 1 } } */
> +/* { dg-final { scan-assembler-times {\mstxvrdx\M} 1 } } */
