Hi Will,
Some comments are inline.
on 2022/9/20 00:05, will schmidt wrote:
> [PATCH, rs6000] Tests of ARCH_PWR8 and -mno-vsx option.
>
> Hi,
>
> This adds an assortment of tests to exercise the -mno-vsx option and
> confirm the impacts on the ARCH_PWR8 define.
>
> These are based on and inspired by PR 101865, which
> reports that _ARCH_PWR8 is disabled when -mno-vsx
> is passed on the commandline.
>
> There are a small number of failures introduced by these tests,
> those are resolved with the changes in part 2.
>
> OK for trunk?
> Thanks,
> -Will
>
>
> gcc/testsuite:
> * gcc.target/powerpc/predefine_p7-novsx.c: New test.
> * gcc.target/powerpc/predefine_p8-noaltivec-novsx.c: New test.
> * gcc.target/powerpc/predefine_p8-novsx.c: New test.
> * gcc.target/powerpc/predefine_p9-novsx.c: New test.
> * gcc.target/powerpc/predefine_pragma_vsx.c: New test.
>
>
> diff --git a/gcc/testsuite/gcc.target/powerpc/predefine_p7-novsx.c
> b/gcc/testsuite/gcc.target/powerpc/predefine_p7-novsx.c
> new file mode 100644
> index 000000000000..e842025b4d3c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/predefine_p7-novsx.c
> @@ -0,0 +1,9 @@
> +/* { dg-do preprocess } */
> +/* Test whether the ARCH_PWR7 and ARCH_PWR8 defines gets set
Nit: s/gets/get.
> + * when we specify power7, plus options.
> +/* This is a variation of the test at issue in GCC PR 101865 */
> +/* { dg-options "-dM -E -mdejagnu-cpu=power7 -mno-vsx" } */
> +/* { dg-final { scan-file predefine_p7-novsx.i "(^|\\n)#define _ARCH_PWR7
> 1($|\\n)" } } */
> +/* { dg-final { scan-file-not predefine_p7-novsx.i "(^|\\n)#define
> _ARCH_PWR8 1($|\\n)" } } */
> +/* { dg-final { scan-file-not predefine_p7-novsx.i "(^|\\n)#define __VSX__
> 1($|\\n)" } } */
> +/* { dg-final { scan-file predefine_p7-novsx.i "(^|\\n)#define __ALTIVEC__
> 1($|\\n)" } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/predefine_p8-noaltivec-novsx.c
> b/gcc/testsuite/gcc.target/powerpc/predefine_p8-noaltivec-novsx.c
> new file mode 100644
> index 000000000000..c3b705ca3d48
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/predefine_p8-noaltivec-novsx.c
> @@ -0,0 +1,7 @@
> +/* { dg-do preprocess } */
> +/* Test whether the ARCH_PWR8 define remains set after disabling both
> altivec and vsx. */
> +/* { dg-options "-dM -E -mdejagnu-cpu=power8 -mno-altivec -mno-vsx" } */
> +/* { dg-final { scan-file predefine_p8-noaltivec-novsx.i "(^|\\n)#define
> _ARCH_PWR8 1($|\\n)" } } */
> +/* { dg-final { scan-file-not predefine_p8-noaltivec-novsx.i "(^|\\n)#define
> _ARCH_PWR9 1($|\\n)" } } */
> +/* { dg-final { scan-file-not predefine_p8-noaltivec-novsx.i "(^|\\n)#define
> __VSX__ 1($|\\n)" } } */
> +/* { dg-final { scan-file-not predefine_p8-noaltivec-novsx.i "(^|\\n)#define
> __ALTIVEC__ 1($|\\n)" } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/predefine_p8-novsx.c
> b/gcc/testsuite/gcc.target/powerpc/predefine_p8-novsx.c
> new file mode 100644
> index 000000000000..8b6c69b20104
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/predefine_p8-novsx.c
> @@ -0,0 +1,8 @@
> +/* { dg-do preprocess } */
> +/* Test whether the ARCH_PWR8 define remains set after disabling vsx.
> + This also confirms __ALTIVEC__ remains set when VSX is disabled. */
> +/* This is the primary test at issue in GCC PR 101865 */
Nit: the last comment missing a period.
> +/* { dg-options "-dM -E -mdejagnu-cpu=power8 -mno-vsx" } */
> +/* { dg-final { scan-file predefine_p8-novsx.i "(^|\\n)#define _ARCH_PWR8
> 1($|\\n)" } } */
> +/* { dg-final { scan-file-not predefine_p8-novsx.i "(^|\\n)#define __VSX__
> 1($|\\n)" } } */
> +/* { dg-final { scan-file predefine_p8-novsx.i "(^|\\n)#define __ALTIVEC__
> 1($|\\n)" } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/predefine_p9-novsx.c
> b/gcc/testsuite/gcc.target/powerpc/predefine_p9-novsx.c
> new file mode 100644
> index 000000000000..eef42c111663
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/predefine_p9-novsx.c
> @@ -0,0 +1,10 @@
> +/* { dg-do preprocess } */
> +/* Test whether the ARCH_PWR8 define remains set after disabling vsx.
> + This also confirms __ALTIVEC__ remains set when VSX is disabled. */
> +/* This is the primary test at issue in GCC PR 101865 */
Nit: it seems this part of comments were copied from the above case?
better with "s/ARCH_PWR8 define/ARCH_PWR8 and ARCH_PWR9 defines/" and
and removing the last sentence since power9 isn't the primary test?
> +/* { dg-options "-dM -E -mdejagnu-cpu=power9 -mno-vsx" } */
> +/* {xfail *-*-*} */
> +/* { dg-final { scan-file predefine_p9-novsx.i "(^|\\n)#define _ARCH_PWR8
> 1($|\\n)" } } */
> +/* { dg-final { scan-file predefine_p9-novsx.i "(^|\\n)#define _ARCH_PWR9
> 1($|\\n)" } } */
> +/* { dg-final { scan-file-not predefine_p9-novsx.i "(^|\\n)#define __VSX__
> 1($|\\n)" } } */
> +/* { dg-final { scan-file predefine_p9-novsx.i "(^|\\n)#define __ALTIVEC__
> 1($|\\n)" } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/predefine_pragma_vsx.c
> b/gcc/testsuite/gcc.target/powerpc/predefine_pragma_vsx.c
> new file mode 100644
> index 000000000000..b300600af999
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/predefine_pragma_vsx.c
> @@ -0,0 +1,83 @@
> +/* { dg-do run } */
> +/* { dg-require-effective-target powerpc_p9vector_ok } */
I guess you meant to use "p8vector_ok"? Ideally we need a power8_hw
here since we force to use -mdejagnu-cpu=power8 below, there could be
some actual p8 insns generated, but as the functions are very very
simple, I don't think it's possible. :)
> +/* { dg-require-effective-target lp64 } */
Is there a particular reason to exclude 32 bit here?
> +/* { dg-options "-mdejagnu-cpu=power8 -mvsx -O2" } */
> +
> +/* Ensure that if we set a pragma gcc target for an
> + older processor, we do not compile builtins that
> + the older target does not support. */
> +
Nit: The comments here seem inconsistent to the contents
below, I guessed it's stale (you used bifs but switched
to check variable values later.)?
BR,
Kewen
> +#include <altivec.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +volatile int power8_set;
> +volatile int vsx_set;
> +
> +void reset_values() {
> + vsx_set=0;
> + power8_set=0;
> +}
> +
> +void test_default() {
> + reset_values();
> +#ifdef _ARCH_PWR8
> + power8_set=1;
> +#endif
> +#ifdef __VSX__
> + vsx_set=1;
> +#endif
> +}
> +
> +#pragma GCC target "no-vsx"
> +void test_no_vsx() {
> + reset_values();
> +#ifdef _ARCH_PWR8
> + power8_set=1;
> +#endif
> +#ifdef __VSX__
> + vsx_set=1;
> +#endif
> +}
> +
> +#pragma GCC reset_options
> +void test_reset_options() {
> + reset_values();
> +#ifdef _ARCH_PWR8
> + power8_set=1;
> +#endif
> +#ifdef __VSX__
> + vsx_set=1;
> +#endif
> +}
> +
> +int main (int argc, char *argv []) {
> + test_default();
> + if (!power8_set) {
> + printf("_ARCH_PWR8 is not set.\n");
> + abort();
> + }
> + if (!vsx_set) {
> + printf("__VSX__ is not set.\n");
> + abort();
> + }
> + test_no_vsx();
> + if (!power8_set) {
> + printf("_ARCH_PWR8 is not set.\n");
> + abort();
> + }
> + if (vsx_set) {
> + printf("__VSX__ is unexpectedly set.\n");
> + abort();
> + }
> + test_reset_options();
> + if (!power8_set) {
> + printf("_ARCH_PWR8 is not set.\n");
> + abort();
> + }
> + if (!vsx_set) {
> + printf("__VSX__ is not set.\n");
> + abort();
> + }
> +}
> +
>
