[PATCH, rs6000] Tests of ARCH_PWR8 and -mno-vsx option. (1/2)

[PATCH, rs6000] Tests of ARCH_PWR8 and -mno-vsx option. (1/2)

[will schmidt]

[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
+ * 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 */
+/* { 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 */
+/* { 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 } */
+/* { dg-require-effective-target lp64 } */
+/* { 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.  */
+
+#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();
+	}
+}
+

[PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2)

[will schmidt]

[PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865]

Hi,
  The _ARCH_PWR8 define is conditional on TARGET_DIRECT_MOVE,
and can be disabled by dependent options when it should not be.
This manifests in the issue seen in PR101865 where -mno-vsx
mistakenly disables _ARCH_PWR8.

This change replaces the relevant TARGET_DIRECT_MOVE references
with a TARGET_POWER8 entry so that the direct_move and power8
features can be enabled or disabled independently.

This is done via the OPTION_MASK definitions, so this
means that some references to the OPTION_MASK_DIRECT_MOVE
option are now replaced with OPTION_MASK_POWER8.

The existing (and rather lengthy) commentary for DIRECT_MOVE remains
in place in rs6000-c.cc:rs6000_target_modify_macros().  The
if-defined logic there will now set a __DIRECT_MOVE__ define when
TARGET_DIRECT_MOVE is set, this serves as a placeholder for debug
purposes, but is otherwise unused.  This can be removed in a
subsequent patch, or in an update of this patch, depending on feedback.

This regests cleanly (power8,power9,power10), and resolves
PR 101865 as represented in the tests from (1/2).

OK for trunk?
Thanks,
-Will


gcc/
	PR Target/101865
	* config/rs6000/rs6000-builtin.cc
	(rs6000_builtin_is_supported): Replace TARGET_DIRECT_MOVE
	usage with TARGET_POWER8.
	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros):
	Add __DIRECT_MOVE__ define.  Replace _ARCH_PWR8_ define
	conditional with OPTION_MASK_POWER8.
	* config/rs6000/rs6000-cpus.def (ISA_2_7_MASKS_SERVER):
	Add OPTION_MASK_POWER8 entry.
	(POWERPC_MASKS): Same.
	* config/rs6000/rs6000.cc (rs6000_option_override_internal):
	Replace OPTION_MASK_DIRECT_MOVE usage with OPTION_MASK_POWER8.
	(rs6000_opt_masks): Add "power8" entry for new OPTION_MASK_POWER8.
	* config/rs6000/rs6000.opt (-mpower8): Add entry for POWER8.
	* config/rs6000/vsx.md (vsx_extract_<mode>): Replace
	TARGET_DIRECT_MOVE usage with TARGET_POWER8.
	(define_peephole2): Same.

diff --git a/gcc/config/rs6000/rs6000-builtin.cc b/gcc/config/rs6000/rs6000-builtin.cc
index 3ce729c1e6de..91a0f39bd796 100644
--- a/gcc/config/rs6000/rs6000-builtin.cc
+++ b/gcc/config/rs6000/rs6000-builtin.cc
@@ -163,11 +163,11 @@ rs6000_builtin_is_supported (enum rs6000_gen_builtins fncode)
     case ENB_P7:
       return TARGET_POPCNTD;
     case ENB_P7_64:
       return TARGET_POPCNTD && TARGET_POWERPC64;
     case ENB_P8:
-      return TARGET_DIRECT_MOVE;
+      return TARGET_POWER8;
     case ENB_P8V:
       return TARGET_P8_VECTOR;
     case ENB_P9:
       return TARGET_MODULO;
     case ENB_P9_64:
diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
index ca9cc42028f7..41d51b039061 100644
--- a/gcc/config/rs6000/rs6000-c.cc
+++ b/gcc/config/rs6000/rs6000-c.cc
@@ -439,11 +439,13 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags)
      turned off in any of the following conditions:
      1. TARGET_HARD_FLOAT, TARGET_ALTIVEC, or TARGET_VSX is explicitly
 	disabled and OPTION_MASK_DIRECT_MOVE was not explicitly
 	enabled.
      2. TARGET_VSX is off.  */
-  if ((flags & OPTION_MASK_DIRECT_MOVE) != 0)
+  if ((OPTION_MASK_DIRECT_MOVE) != 0)
+    rs6000_define_or_undefine_macro (define_p, "__DIRECT_MOVE__");
+  if ((flags & OPTION_MASK_POWER8) != 0)
     rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR8");
   if ((flags & OPTION_MASK_MODULO) != 0)
     rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR9");
   if ((flags & OPTION_MASK_POWER10) != 0)
     rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR10");
diff --git a/gcc/config/rs6000/rs6000-cpus.def b/gcc/config/rs6000/rs6000-cpus.def
index c3825bcccd84..c873f6d58989 100644
--- a/gcc/config/rs6000/rs6000-cpus.def
+++ b/gcc/config/rs6000/rs6000-cpus.def
@@ -48,10 +48,11 @@
    system.  */
 #define ISA_2_7_MASKS_SERVER	(ISA_2_6_MASKS_SERVER			\
 				 | OPTION_MASK_P8_VECTOR		\
 				 | OPTION_MASK_CRYPTO			\
 				 | OPTION_MASK_DIRECT_MOVE		\
+				 | OPTION_MASK_POWER8			\
 				 | OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
 				 | OPTION_MASK_QUAD_MEMORY		\
 				 | OPTION_MASK_QUAD_MEMORY_ATOMIC)
 
 /* ISA masks setting fusion options.  */
@@ -124,10 +125,11 @@
 #define POWERPC_MASKS		(OPTION_MASK_ALTIVEC			\
 				 | OPTION_MASK_CMPB			\
 				 | OPTION_MASK_CRYPTO			\
 				 | OPTION_MASK_DFP			\
 				 | OPTION_MASK_DIRECT_MOVE		\
+				 | OPTION_MASK_POWER8			\
 				 | OPTION_MASK_DLMZB			\
 				 | OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
 				 | OPTION_MASK_FLOAT128_HW		\
 				 | OPTION_MASK_FLOAT128_KEYWORD		\
 				 | OPTION_MASK_FPRND			\
diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
index fcca062a8709..ed423b9e1837 100644
--- a/gcc/config/rs6000/rs6000.cc
+++ b/gcc/config/rs6000/rs6000.cc
@@ -3766,15 +3766,14 @@ rs6000_option_override_internal (bool global_init_p)
       if ((rs6000_isa_flags_explicit & OPTION_MASK_MULTIPLE) != 0)
 	warning (0, "%qs is not supported on little endian systems",
 		 "-mmultiple");
     }
 
-  /* If little-endian, default to -mstrict-align on older processors.
-     Testing for direct_move matches power8 and later.  */
+  /* If little-endian, default to -mstrict-align on older processors.  */
   if (!BYTES_BIG_ENDIAN
       && !(processor_target_table[tune_index].target_enable
-	   & OPTION_MASK_DIRECT_MOVE))
+	   & OPTION_MASK_POWER8))
     rs6000_isa_flags |= ~rs6000_isa_flags_explicit & OPTION_MASK_STRICT_ALIGN;
 
   /* Add some warnings for VSX.  */
   if (TARGET_VSX)
     {
@@ -3857,11 +3856,11 @@ rs6000_option_override_internal (bool global_init_p)
 	error ("%qs incompatible with explicitly disabled options",
 	       "-mpower9-minmax");
       else
 	rs6000_isa_flags |= ISA_3_0_MASKS_SERVER;
     }
-  else if (TARGET_P8_VECTOR || TARGET_DIRECT_MOVE || TARGET_CRYPTO)
+  else if (TARGET_P8_VECTOR || TARGET_POWER8 || TARGET_CRYPTO)
     rs6000_isa_flags |= (ISA_2_7_MASKS_SERVER & ~ignore_masks);
   else if (TARGET_VSX)
     rs6000_isa_flags |= (ISA_2_6_MASKS_SERVER & ~ignore_masks);
   else if (TARGET_POPCNTD)
     rs6000_isa_flags |= (ISA_2_6_MASKS_EMBEDDED & ~ignore_masks);
@@ -24046,10 +24045,11 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =
   { "block-ops-vector-pair",	OPTION_MASK_BLOCK_OPS_VECTOR_PAIR,
 								false, true  },
   { "cmpb",			OPTION_MASK_CMPB,		false, true  },
   { "crypto",			OPTION_MASK_CRYPTO,		false, true  },
   { "direct-move",		OPTION_MASK_DIRECT_MOVE,	false, true  },
+  { "power8",			OPTION_MASK_POWER8,		false, true  },
   { "dlmzb",			OPTION_MASK_DLMZB,		false, true  },
   { "efficient-unaligned-vsx",	OPTION_MASK_EFFICIENT_UNALIGNED_VSX,
 								false, true  },
   { "float128",			OPTION_MASK_FLOAT128_KEYWORD,	false, true  },
   { "float128-hardware",	OPTION_MASK_FLOAT128_HW,	false, true  },
diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
index b63a5d443af6..53964387da6d 100644
--- a/gcc/config/rs6000/rs6000.opt
+++ b/gcc/config/rs6000/rs6000.opt
@@ -490,10 +490,15 @@ mcrypto
 Target Mask(CRYPTO) Var(rs6000_isa_flags)
 Use ISA 2.07 Category:Vector.AES and Category:Vector.SHA2 instructions.
 
 mdirect-move
 Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
+Enable direct move (ISA 2.07).
+
+mpower8
+Target Mask(POWER8) Var(rs6000_isa_flags)
+Use instructions added in ISA 2.07 (power8).
 
 mhtm
 Target Mask(HTM) Var(rs6000_isa_flags)
 Use ISA 2.07 transactional memory (HTM) instructions.
 
diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
index e226a93bbe55..be4fb902049d 100644
--- a/gcc/config/rs6000/vsx.md
+++ b/gcc/config/rs6000/vsx.md
@@ -3407,11 +3407,11 @@ (define_insn "vsx_extract_<mode>"
   if (element == VECTOR_ELEMENT_SCALAR_64BIT)
     {
       if (op0_regno == op1_regno)
 	return ASM_COMMENT_START " vec_extract to same register";
 
-      else if (INT_REGNO_P (op0_regno) && TARGET_DIRECT_MOVE
+      else if (INT_REGNO_P (op0_regno) && TARGET_POWER8
 	       && TARGET_POWERPC64)
 	return "mfvsrd %0,%x1";
 
       else if (FP_REGNO_P (op0_regno) && FP_REGNO_P (op1_regno))
 	return "fmr %0,%1";
@@ -6204,11 +6204,11 @@ (define_peephole2
 
    ;; MTVSRD
    (set (match_operand:SF SFBOOL_MTVSR_D "vsx_register_operand")
 	(unspec:SF [(match_dup SFBOOL_SHL_D)] UNSPEC_P8V_MTVSRD))]
 
-  "TARGET_POWERPC64 && TARGET_DIRECT_MOVE
+  "TARGET_POWERPC64 && TARGET_POWER8
    /* The REG_P (xxx) tests prevents SUBREG's, which allows us to use REGNO
       to compare registers, when the mode is different.  */
    && REG_P (operands[SFBOOL_MFVSR_D]) && REG_P (operands[SFBOOL_BOOL_D])
    && REG_P (operands[SFBOOL_BOOL_A1]) && REG_P (operands[SFBOOL_SHL_D])
    && REG_P (operands[SFBOOL_SHL_A])   && REG_P (operands[SFBOOL_MTVSR_D])

Re: [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2)

[will schmidt]

Ping.

On Mon, 2022-09-19 at 11:13 -0500, will schmidt wrote:
> [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865]
> 
> Hi,
>   The _ARCH_PWR8 define is conditional on TARGET_DIRECT_MOVE,
> and can be disabled by dependent options when it should not be.
> This manifests in the issue seen in PR101865 where -mno-vsx
> mistakenly disables _ARCH_PWR8.
> 
> This change replaces the relevant TARGET_DIRECT_MOVE references
> with a TARGET_POWER8 entry so that the direct_move and power8
> features can be enabled or disabled independently.
> 
> This is done via the OPTION_MASK definitions, so this
> means that some references to the OPTION_MASK_DIRECT_MOVE
> option are now replaced with OPTION_MASK_POWER8.
> 
> The existing (and rather lengthy) commentary for DIRECT_MOVE remains
> in place in rs6000-c.cc:rs6000_target_modify_macros().  The
> if-defined logic there will now set a __DIRECT_MOVE__ define when
> TARGET_DIRECT_MOVE is set, this serves as a placeholder for debug
> purposes, but is otherwise unused.  This can be removed in a
> subsequent patch, or in an update of this patch, depending on feedback.
> 
> This regests cleanly (power8,power9,power10), and resolves
> PR 101865 as represented in the tests from (1/2).
> 
> OK for trunk?
> Thanks,
> -Will
> 
> 
> gcc/
> 	PR Target/101865
> 	* config/rs6000/rs6000-builtin.cc
> 	(rs6000_builtin_is_supported): Replace TARGET_DIRECT_MOVE
> 	usage with TARGET_POWER8.
> 	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros):
> 	Add __DIRECT_MOVE__ define.  Replace _ARCH_PWR8_ define
> 	conditional with OPTION_MASK_POWER8.
> 	* config/rs6000/rs6000-cpus.def (ISA_2_7_MASKS_SERVER):
> 	Add OPTION_MASK_POWER8 entry.
> 	(POWERPC_MASKS): Same.
> 	* config/rs6000/rs6000.cc (rs6000_option_override_internal):
> 	Replace OPTION_MASK_DIRECT_MOVE usage with OPTION_MASK_POWER8.
> 	(rs6000_opt_masks): Add "power8" entry for new OPTION_MASK_POWER8.
> 	* config/rs6000/rs6000.opt (-mpower8): Add entry for POWER8.
> 	* config/rs6000/vsx.md (vsx_extract_<mode>): Replace
> 	TARGET_DIRECT_MOVE usage with TARGET_POWER8.
> 	(define_peephole2): Same.
> 
> diff --git a/gcc/config/rs6000/rs6000-builtin.cc b/gcc/config/rs6000/rs6000-builtin.cc
> index 3ce729c1e6de..91a0f39bd796 100644
> --- a/gcc/config/rs6000/rs6000-builtin.cc
> +++ b/gcc/config/rs6000/rs6000-builtin.cc
> @@ -163,11 +163,11 @@ rs6000_builtin_is_supported (enum rs6000_gen_builtins fncode)
>      case ENB_P7:
>        return TARGET_POPCNTD;
>      case ENB_P7_64:
>        return TARGET_POPCNTD && TARGET_POWERPC64;
>      case ENB_P8:
> -      return TARGET_DIRECT_MOVE;
> +      return TARGET_POWER8;
>      case ENB_P8V:
>        return TARGET_P8_VECTOR;
>      case ENB_P9:
>        return TARGET_MODULO;
>      case ENB_P9_64:
> diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
> index ca9cc42028f7..41d51b039061 100644
> --- a/gcc/config/rs6000/rs6000-c.cc
> +++ b/gcc/config/rs6000/rs6000-c.cc
> @@ -439,11 +439,13 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags)
>       turned off in any of the following conditions:
>       1. TARGET_HARD_FLOAT, TARGET_ALTIVEC, or TARGET_VSX is explicitly
>  	disabled and OPTION_MASK_DIRECT_MOVE was not explicitly
>  	enabled.
>       2. TARGET_VSX is off.  */
> -  if ((flags & OPTION_MASK_DIRECT_MOVE) != 0)
> +  if ((OPTION_MASK_DIRECT_MOVE) != 0)
> +    rs6000_define_or_undefine_macro (define_p, "__DIRECT_MOVE__");
> +  if ((flags & OPTION_MASK_POWER8) != 0)
>      rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR8");
>    if ((flags & OPTION_MASK_MODULO) != 0)
>      rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR9");
>    if ((flags & OPTION_MASK_POWER10) != 0)
>      rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR10");
> diff --git a/gcc/config/rs6000/rs6000-cpus.def b/gcc/config/rs6000/rs6000-cpus.def
> index c3825bcccd84..c873f6d58989 100644
> --- a/gcc/config/rs6000/rs6000-cpus.def
> +++ b/gcc/config/rs6000/rs6000-cpus.def
> @@ -48,10 +48,11 @@
>     system.  */
>  #define ISA_2_7_MASKS_SERVER	(ISA_2_6_MASKS_SERVER			\
>  				 | OPTION_MASK_P8_VECTOR		\
>  				 | OPTION_MASK_CRYPTO			\
>  				 | OPTION_MASK_DIRECT_MOVE		\
> +				 | OPTION_MASK_POWER8			\
>  				 | OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
>  				 | OPTION_MASK_QUAD_MEMORY		\
>  				 | OPTION_MASK_QUAD_MEMORY_ATOMIC)
> 
>  /* ISA masks setting fusion options.  */
> @@ -124,10 +125,11 @@
>  #define POWERPC_MASKS		(OPTION_MASK_ALTIVEC			\
>  				 | OPTION_MASK_CMPB			\
>  				 | OPTION_MASK_CRYPTO			\
>  				 | OPTION_MASK_DFP			\
>  				 | OPTION_MASK_DIRECT_MOVE		\
> +				 | OPTION_MASK_POWER8			\
>  				 | OPTION_MASK_DLMZB			\
>  				 | OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
>  				 | OPTION_MASK_FLOAT128_HW		\
>  				 | OPTION_MASK_FLOAT128_KEYWORD		\
>  				 | OPTION_MASK_FPRND			\
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index fcca062a8709..ed423b9e1837 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -3766,15 +3766,14 @@ rs6000_option_override_internal (bool global_init_p)
>        if ((rs6000_isa_flags_explicit & OPTION_MASK_MULTIPLE) != 0)
>  	warning (0, "%qs is not supported on little endian systems",
>  		 "-mmultiple");
>      }
> 
> -  /* If little-endian, default to -mstrict-align on older processors.
> -     Testing for direct_move matches power8 and later.  */
> +  /* If little-endian, default to -mstrict-align on older processors.  */
>    if (!BYTES_BIG_ENDIAN
>        && !(processor_target_table[tune_index].target_enable
> -	   & OPTION_MASK_DIRECT_MOVE))
> +	   & OPTION_MASK_POWER8))
>      rs6000_isa_flags |= ~rs6000_isa_flags_explicit & OPTION_MASK_STRICT_ALIGN;
> 
>    /* Add some warnings for VSX.  */
>    if (TARGET_VSX)
>      {
> @@ -3857,11 +3856,11 @@ rs6000_option_override_internal (bool global_init_p)
>  	error ("%qs incompatible with explicitly disabled options",
>  	       "-mpower9-minmax");
>        else
>  	rs6000_isa_flags |= ISA_3_0_MASKS_SERVER;
>      }
> -  else if (TARGET_P8_VECTOR || TARGET_DIRECT_MOVE || TARGET_CRYPTO)
> +  else if (TARGET_P8_VECTOR || TARGET_POWER8 || TARGET_CRYPTO)
>      rs6000_isa_flags |= (ISA_2_7_MASKS_SERVER & ~ignore_masks);
>    else if (TARGET_VSX)
>      rs6000_isa_flags |= (ISA_2_6_MASKS_SERVER & ~ignore_masks);
>    else if (TARGET_POPCNTD)
>      rs6000_isa_flags |= (ISA_2_6_MASKS_EMBEDDED & ~ignore_masks);
> @@ -24046,10 +24045,11 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =
>    { "block-ops-vector-pair",	OPTION_MASK_BLOCK_OPS_VECTOR_PAIR,
>  								false, true  },
>    { "cmpb",			OPTION_MASK_CMPB,		false, true  },
>    { "crypto",			OPTION_MASK_CRYPTO,		false, true  },
>    { "direct-move",		OPTION_MASK_DIRECT_MOVE,	false, true  },
> +  { "power8",			OPTION_MASK_POWER8,		false, true  },
>    { "dlmzb",			OPTION_MASK_DLMZB,		false, true  },
>    { "efficient-unaligned-vsx",	OPTION_MASK_EFFICIENT_UNALIGNED_VSX,
>  								false, true  },
>    { "float128",			OPTION_MASK_FLOAT128_KEYWORD,	false, true  },
>    { "float128-hardware",	OPTION_MASK_FLOAT128_HW,	false, true  },
> diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
> index b63a5d443af6..53964387da6d 100644
> --- a/gcc/config/rs6000/rs6000.opt
> +++ b/gcc/config/rs6000/rs6000.opt
> @@ -490,10 +490,15 @@ mcrypto
>  Target Mask(CRYPTO) Var(rs6000_isa_flags)
>  Use ISA 2.07 Category:Vector.AES and Category:Vector.SHA2 instructions.
> 
>  mdirect-move
>  Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
> +Enable direct move (ISA 2.07).
> +
> +mpower8
> +Target Mask(POWER8) Var(rs6000_isa_flags)
> +Use instructions added in ISA 2.07 (power8).
> 
>  mhtm
>  Target Mask(HTM) Var(rs6000_isa_flags)
>  Use ISA 2.07 transactional memory (HTM) instructions.
> 
> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
> index e226a93bbe55..be4fb902049d 100644
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -3407,11 +3407,11 @@ (define_insn "vsx_extract_<mode>"
>    if (element == VECTOR_ELEMENT_SCALAR_64BIT)
>      {
>        if (op0_regno == op1_regno)
>  	return ASM_COMMENT_START " vec_extract to same register";
> 
> -      else if (INT_REGNO_P (op0_regno) && TARGET_DIRECT_MOVE
> +      else if (INT_REGNO_P (op0_regno) && TARGET_POWER8
>  	       && TARGET_POWERPC64)
>  	return "mfvsrd %0,%x1";
> 
>        else if (FP_REGNO_P (op0_regno) && FP_REGNO_P (op1_regno))
>  	return "fmr %0,%1";
> @@ -6204,11 +6204,11 @@ (define_peephole2
> 
>     ;; MTVSRD
>     (set (match_operand:SF SFBOOL_MTVSR_D "vsx_register_operand")
>  	(unspec:SF [(match_dup SFBOOL_SHL_D)] UNSPEC_P8V_MTVSRD))]
> 
> -  "TARGET_POWERPC64 && TARGET_DIRECT_MOVE
> +  "TARGET_POWERPC64 && TARGET_POWER8
>     /* The REG_P (xxx) tests prevents SUBREG's, which allows us to use REGNO
>        to compare registers, when the mode is different.  */
>     && REG_P (operands[SFBOOL_MFVSR_D]) && REG_P (operands[SFBOOL_BOOL_D])
>     && REG_P (operands[SFBOOL_BOOL_A1]) && REG_P (operands[SFBOOL_SHL_D])
>     && REG_P (operands[SFBOOL_SHL_A])   && REG_P (operands[SFBOOL_MTVSR_D])
> 

Re: [PATCH, rs6000] Tests of ARCH_PWR8 and -mno-vsx option. (1/2)

[Kewen.Lin]

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();
> +	}
> +}
> +
> 

Re: [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2)

[Kewen.Lin]

Hi Will,

Thanks for fixing this, some comments are inline as below.

on 2022/9/20 00:13, will schmidt wrote:
> [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865]
> 
> Hi,
>   The _ARCH_PWR8 define is conditional on TARGET_DIRECT_MOVE,
> and can be disabled by dependent options when it should not be.
> This manifests in the issue seen in PR101865 where -mno-vsx
> mistakenly disables _ARCH_PWR8.
> 
> This change replaces the relevant TARGET_DIRECT_MOVE references
> with a TARGET_POWER8 entry so that the direct_move and power8
> features can be enabled or disabled independently.
> 
> This is done via the OPTION_MASK definitions, so this
> means that some references to the OPTION_MASK_DIRECT_MOVE
> option are now replaced with OPTION_MASK_POWER8.
> 
> The existing (and rather lengthy) commentary for DIRECT_MOVE remains
> in place in rs6000-c.cc:rs6000_target_modify_macros().  The
> if-defined logic there will now set a __DIRECT_MOVE__ define when
> TARGET_DIRECT_MOVE is set, this serves as a placeholder for debug
> purposes, but is otherwise unused.  This can be removed in a
> subsequent patch, or in an update of this patch, depending on feedback.

The mentioned commentary for DIRECT_MOVE looks out of date since
option direct_move is marked as Undocumented & WarnRemoved, it can't
be enabled/disabled explicitly.  Personally I'm inclined not to
introduce __DIRECT_MOVE__ define, since we don't have a separated
option for it now, and if users want to check the availability,
they can check __VSX__ && _ARCH_PWR8 instead.

> 
> This regests cleanly (power8,power9,power10), and resolves
> PR 101865 as represented in the tests from (1/2).
> 
> OK for trunk?
> Thanks,
> -Will
> 
> 
> gcc/
> 	PR Target/101865
> 	* config/rs6000/rs6000-builtin.cc
> 	(rs6000_builtin_is_supported): Replace TARGET_DIRECT_MOVE
> 	usage with TARGET_POWER8.
> 	* config/rs6000/rs6000-c.cc (rs6000_target_modify_macros):
> 	Add __DIRECT_MOVE__ define.  Replace _ARCH_PWR8_ define
> 	conditional with OPTION_MASK_POWER8.
> 	* config/rs6000/rs6000-cpus.def (ISA_2_7_MASKS_SERVER):
> 	Add OPTION_MASK_POWER8 entry.
> 	(POWERPC_MASKS): Same.
> 	* config/rs6000/rs6000.cc (rs6000_option_override_internal):
> 	Replace OPTION_MASK_DIRECT_MOVE usage with OPTION_MASK_POWER8.
> 	(rs6000_opt_masks): Add "power8" entry for new OPTION_MASK_POWER8.
> 	* config/rs6000/rs6000.opt (-mpower8): Add entry for POWER8.
> 	* config/rs6000/vsx.md (vsx_extract_<mode>): Replace
> 	TARGET_DIRECT_MOVE usage with TARGET_POWER8.
> 	(define_peephole2): Same.
> 
> diff --git a/gcc/config/rs6000/rs6000-builtin.cc b/gcc/config/rs6000/rs6000-builtin.cc
> index 3ce729c1e6de..91a0f39bd796 100644
> --- a/gcc/config/rs6000/rs6000-builtin.cc
> +++ b/gcc/config/rs6000/rs6000-builtin.cc
> @@ -163,11 +163,11 @@ rs6000_builtin_is_supported (enum rs6000_gen_builtins fncode)
>      case ENB_P7:
>        return TARGET_POPCNTD;
>      case ENB_P7_64:
>        return TARGET_POPCNTD && TARGET_POWERPC64;
>      case ENB_P8:
> -      return TARGET_DIRECT_MOVE;
> +      return TARGET_POWER8;
>      case ENB_P8V:
>        return TARGET_P8_VECTOR;
>      case ENB_P9:
>        return TARGET_MODULO;
>      case ENB_P9_64:
> diff --git a/gcc/config/rs6000/rs6000-c.cc b/gcc/config/rs6000/rs6000-c.cc
> index ca9cc42028f7..41d51b039061 100644
> --- a/gcc/config/rs6000/rs6000-c.cc
> +++ b/gcc/config/rs6000/rs6000-c.cc
> @@ -439,11 +439,13 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags)
>       turned off in any of the following conditions:
>       1. TARGET_HARD_FLOAT, TARGET_ALTIVEC, or TARGET_VSX is explicitly
>  	disabled and OPTION_MASK_DIRECT_MOVE was not explicitly
>  	enabled.
>       2. TARGET_VSX is off.  */

As mentioned above, the comments might need some updates.

> -  if ((flags & OPTION_MASK_DIRECT_MOVE) != 0)
> +  if ((OPTION_MASK_DIRECT_MOVE) != 0)
> +    rs6000_define_or_undefine_macro (define_p, "__DIRECT_MOVE__");
> +  if ((flags & OPTION_MASK_POWER8) != 0)
>      rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR8");
>    if ((flags & OPTION_MASK_MODULO) != 0)
>      rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR9");
>    if ((flags & OPTION_MASK_POWER10) != 0)
>      rs6000_define_or_undefine_macro (define_p, "_ARCH_PWR10");
> diff --git a/gcc/config/rs6000/rs6000-cpus.def b/gcc/config/rs6000/rs6000-cpus.def
> index c3825bcccd84..c873f6d58989 100644
> --- a/gcc/config/rs6000/rs6000-cpus.def
> +++ b/gcc/config/rs6000/rs6000-cpus.def
> @@ -48,10 +48,11 @@
>     system.  */
>  #define ISA_2_7_MASKS_SERVER	(ISA_2_6_MASKS_SERVER			\
>  				 | OPTION_MASK_P8_VECTOR		\
>  				 | OPTION_MASK_CRYPTO			\
>  				 | OPTION_MASK_DIRECT_MOVE		\
> +				 | OPTION_MASK_POWER8			\
>  				 | OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
>  				 | OPTION_MASK_QUAD_MEMORY		\
>  				 | OPTION_MASK_QUAD_MEMORY_ATOMIC)
>  
>  /* ISA masks setting fusion options.  */
> @@ -124,10 +125,11 @@
>  #define POWERPC_MASKS		(OPTION_MASK_ALTIVEC			\
>  				 | OPTION_MASK_CMPB			\
>  				 | OPTION_MASK_CRYPTO			\
>  				 | OPTION_MASK_DFP			\
>  				 | OPTION_MASK_DIRECT_MOVE		\
> +				 | OPTION_MASK_POWER8			\
>  				 | OPTION_MASK_DLMZB			\
>  				 | OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
>  				 | OPTION_MASK_FLOAT128_HW		\
>  				 | OPTION_MASK_FLOAT128_KEYWORD		\
>  				 | OPTION_MASK_FPRND			\
> diff --git a/gcc/config/rs6000/rs6000.cc b/gcc/config/rs6000/rs6000.cc
> index fcca062a8709..ed423b9e1837 100644
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -3766,15 +3766,14 @@ rs6000_option_override_internal (bool global_init_p)
>        if ((rs6000_isa_flags_explicit & OPTION_MASK_MULTIPLE) != 0)
>  	warning (0, "%qs is not supported on little endian systems",
>  		 "-mmultiple");
>      }
>  
> -  /* If little-endian, default to -mstrict-align on older processors.
> -     Testing for direct_move matches power8 and later.  */
> +  /* If little-endian, default to -mstrict-align on older processors.  */
>    if (!BYTES_BIG_ENDIAN
>        && !(processor_target_table[tune_index].target_enable
> -	   & OPTION_MASK_DIRECT_MOVE))
> +	   & OPTION_MASK_POWER8))
>      rs6000_isa_flags |= ~rs6000_isa_flags_explicit & OPTION_MASK_STRICT_ALIGN;
>  
>    /* Add some warnings for VSX.  */
>    if (TARGET_VSX)
>      {
> @@ -3857,11 +3856,11 @@ rs6000_option_override_internal (bool global_init_p)
>  	error ("%qs incompatible with explicitly disabled options",
>  	       "-mpower9-minmax");
>        else
>  	rs6000_isa_flags |= ISA_3_0_MASKS_SERVER;
>      }
> -  else if (TARGET_P8_VECTOR || TARGET_DIRECT_MOVE || TARGET_CRYPTO)
> +  else if (TARGET_P8_VECTOR || TARGET_POWER8 || TARGET_CRYPTO)
>      rs6000_isa_flags |= (ISA_2_7_MASKS_SERVER & ~ignore_masks);
>    else if (TARGET_VSX)
>      rs6000_isa_flags |= (ISA_2_6_MASKS_SERVER & ~ignore_masks);
>    else if (TARGET_POPCNTD)
>      rs6000_isa_flags |= (ISA_2_6_MASKS_EMBEDDED & ~ignore_masks);
> @@ -24046,10 +24045,11 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =
>    { "block-ops-vector-pair",	OPTION_MASK_BLOCK_OPS_VECTOR_PAIR,
>  								false, true  },
>    { "cmpb",			OPTION_MASK_CMPB,		false, true  },
>    { "crypto",			OPTION_MASK_CRYPTO,		false, true  },
>    { "direct-move",		OPTION_MASK_DIRECT_MOVE,	false, true  },
> +  { "power8",			OPTION_MASK_POWER8,		false, true  },
>    { "dlmzb",			OPTION_MASK_DLMZB,		false, true  },
>    { "efficient-unaligned-vsx",	OPTION_MASK_EFFICIENT_UNALIGNED_VSX,
>  								false, true  },
>    { "float128",			OPTION_MASK_FLOAT128_KEYWORD,	false, true  },
>    { "float128-hardware",	OPTION_MASK_FLOAT128_HW,	false, true  },
> diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
> index b63a5d443af6..53964387da6d 100644
> --- a/gcc/config/rs6000/rs6000.opt
> +++ b/gcc/config/rs6000/rs6000.opt
> @@ -490,10 +490,15 @@ mcrypto
>  Target Mask(CRYPTO) Var(rs6000_isa_flags)
>  Use ISA 2.07 Category:Vector.AES and Category:Vector.SHA2 instructions.
>  
>  mdirect-move
>  Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
> +Enable direct move (ISA 2.07).

Since this option is marked as "Undocumented", I think it doesn't need the
documentation here.  And it seems not a good idea to make this option back
settable by users.

> +
> +mpower8
> +Target Mask(POWER8) Var(rs6000_isa_flags)
> +Use instructions added in ISA 2.07 (power8).
>  

We don't allow users to specify -mpower10, so I think we don't want to allow
it for power8 too.  So can we mark it as "Undocumented" and "WarnRemoved" 
(or similar to ensure users can't specify it)?

>  mhtm
>  Target Mask(HTM) Var(rs6000_isa_flags)
>  Use ISA 2.07 transactional memory (HTM) instructions.
>  
> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
> index e226a93bbe55..be4fb902049d 100644
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -3407,11 +3407,11 @@ (define_insn "vsx_extract_<mode>"
>    if (element == VECTOR_ELEMENT_SCALAR_64BIT)
>      {
>        if (op0_regno == op1_regno)
>  	return ASM_COMMENT_START " vec_extract to same register";
>  
> -      else if (INT_REGNO_P (op0_regno) && TARGET_DIRECT_MOVE
> +      else if (INT_REGNO_P (op0_regno) && TARGET_POWER8
>  	       && TARGET_POWERPC64)
>  	return "mfvsrd %0,%x1";

The context here indicates the TARGET_DIRECT_MOVE use is expected here,
as it's moving from VSX register to GPR directly, no?

>  
>        else if (FP_REGNO_P (op0_regno) && FP_REGNO_P (op1_regno))
>  	return "fmr %0,%1";
> @@ -6204,11 +6204,11 @@ (define_peephole2
>  
>     ;; MTVSRD
>     (set (match_operand:SF SFBOOL_MTVSR_D "vsx_register_operand")
>  	(unspec:SF [(match_dup SFBOOL_SHL_D)] UNSPEC_P8V_MTVSRD))]
>  
> -  "TARGET_POWERPC64 && TARGET_DIRECT_MOVE
> +  "TARGET_POWERPC64 && TARGET_POWER8

Similar here, could you have a double check?

>     /* The REG_P (xxx) tests prevents SUBREG's, which allows us to use REGNO
>        to compare registers, when the mode is different.  */
>     && REG_P (operands[SFBOOL_MFVSR_D]) && REG_P (operands[SFBOOL_BOOL_D])
>     && REG_P (operands[SFBOOL_BOOL_A1]) && REG_P (operands[SFBOOL_SHL_D])
>     && REG_P (operands[SFBOOL_SHL_A])   && REG_P (operands[SFBOOL_MTVSR_D])
> 

Besides, one place in rs6000.md that caught my eyes and requires an update
seems to be:

(define_insn "prefetch"
  [(prefetch (match_operand 0 "indexed_or_indirect_address" "a")
	     (match_operand:SI 1 "const_int_operand" "n")
	     (match_operand:SI 2 "const_int_operand" "n"))]
  ""
{

  /* dcbtstt, dcbtt and TH=0b10000 support starts with ISA 2.06 (Power7).
     AIX does not support the dcbtstt and dcbtt extended mnemonics.
     The AIX assembler does not support the three operand form of dcbt
     and dcbtst on Power 7 (-mpwr7).  */
  int inst_select = INTVAL (operands[2]) || !TARGET_DIRECT_MOVE;

As the changelog of the related commit, TARGET_DIRECT_MOVE here is used
to guard power8.
 
BR,
Kewen

Re: [PATCH, rs6000] Tests of ARCH_PWR8 and -mno-vsx option. (1/2)

[Segher Boessenkool]

Hi!

Everything Ke Wen said.  Some more commments / hints:

On Mon, Sep 19, 2022 at 11:05:17AM -0500, will schmidt wrote:
> --- /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
> + * when we specify power7, plus options.
> +/* This is a variation of the test at issue in GCC PR 101865 */

Please don't start comment lines with stars.  And don't start a comment
inside of another comment :-)

> +/* { dg-options "-dM -E -mdejagnu-cpu=power7 -mno-vsx" } */
> +/* { dg-final { scan-file predefine_p7-novsx.i "(^|\\n)#define _ARCH_PWR7 1($|\\n)"  } } */

REs are easier to read and write if you write them inside {} instead of
inside "".

Whenever you see  (^|\n)  it should hint you to use newline-sensitive
matching?  Like
  {(?n)^#define _ARCH_PWR7 1$}
(it makes ^ and $ match the begin/end of lines instead of of the string,
and makes . and similar not match newlines).

> --- /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 */
> +/* { dg-options "-dM -E -mdejagnu-cpu=power9 -mno-vsx" } */
> +/* {xfail *-*-*} */

An xfail always needs a comment :-)


Segher

Re: [PATCH, rs6000] Tests of ARCH_PWR8 and -mno-vsx option. (1/2)

[will schmidt]

On Mon, 2022-10-17 at 10:32 -0500, Segher Boessenkool wrote:
> Hi!
> 
> Everything Ke Wen said.  Some more commments / hints:

Thanks for the reviews. :-)

I'll rework things and repost 'soon'.

Thanks
-WIll

Re: [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2)

[Segher Boessenkool]

On Mon, Sep 19, 2022 at 11:13:20AM -0500, will schmidt wrote:
>   The _ARCH_PWR8 define is conditional on TARGET_DIRECT_MOVE,
> and can be disabled by dependent options when it should not be.
> This manifests in the issue seen in PR101865 where -mno-vsx
> mistakenly disables _ARCH_PWR8.

> This change replaces the relevant TARGET_DIRECT_MOVE references
> with a TARGET_POWER8 entry so that the direct_move and power8
> features can be enabled or disabled independently.

We should get rid of TARGET_DIRECT_MOVE altogether.  Please see
57f108f5a1e1:
    rs6000: Disable -m[no-]direct-move (PR85293)
    
    The -mno-direct-move option causes a lot of problems, since it forces
    us to be able to generate code for p8 and up with some crucial
    instructions missing.  This patch removes the -m[no-]direct-move
    options so that the user cannot put us into this unexpected situation
    anymore.  Internally we still have all the same flags, and they are
    automatically set based on -mcpu; getting rid of that is a lot more
    work and will have to wait for GCC 9 (in some places the flag is used
    to see if we are compiling for a p8 _at all_).

It did not happen in GCC 9 obviously.  Do you want to take a shot?  It
doesn't have to be all at once, it's probably best if not even -- as I
wrote in the commit message, the flag always was used to mean different
things.

> The existing (and rather lengthy) commentary for DIRECT_MOVE remains
> in place in rs6000-c.cc:rs6000_target_modify_macros().  The
> if-defined logic there will now set a __DIRECT_MOVE__ define when
> TARGET_DIRECT_MOVE is set, this serves as a placeholder for debug
> purposes, but is otherwise unused.  This can be removed in a
> subsequent patch, or in an update of this patch, depending on feedback.

There should be no such macro, for the same reason there should be no
-mdirect-move option: it is so very essential to all code we generate,
it *always* is enabled if we have P8 or later.

> gcc/
> 	PR Target/101865
> 	* config/rs6000/rs6000-builtin.cc
> 	(rs6000_builtin_is_supported): Replace TARGET_DIRECT_MOVE
> 	usage with TARGET_POWER8.

Please don't arbitrarily wrap lines.  It is harder to read, and it looks
like something is missing.

> 	* config/rs6000/rs6000-cpus.def (ISA_2_7_MASKS_SERVER):
> 	Add OPTION_MASK_POWER8 entry.

Especially in cases like this, where it looks like you forgot to write
something after the colon.

> @@ -24046,10 +24045,11 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =
>    { "block-ops-vector-pair",	OPTION_MASK_BLOCK_OPS_VECTOR_PAIR,
>  								false, true  },
>    { "cmpb",			OPTION_MASK_CMPB,		false, true  },
>    { "crypto",			OPTION_MASK_CRYPTO,		false, true  },
>    { "direct-move",		OPTION_MASK_DIRECT_MOVE,	false, true  },
> +  { "power8",			OPTION_MASK_POWER8,		false, true  },

Why would we want a #pragma power8 ?

> --- a/gcc/config/rs6000/rs6000.opt
> +++ b/gcc/config/rs6000/rs6000.opt
> @@ -490,10 +490,15 @@ mcrypto
>  Target Mask(CRYPTO) Var(rs6000_isa_flags)
>  Use ISA 2.07 Category:Vector.AES and Category:Vector.SHA2 instructions.
>  
>  mdirect-move
>  Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved
> +Enable direct move (ISA 2.07).

It is undocumented and should remain that, except eventually we should
remove it completely (but leave some stubs so that code in the wild
keeps compiling).

> +mpower8
> +Target Mask(POWER8) Var(rs6000_isa_flags)
> +Use instructions added in ISA 2.07 (power8).

There should not be such an option.  It is set by -mcpu=power8 and
later, but can never be enabled or disabled direfctly by the user.

> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -3407,11 +3407,11 @@ (define_insn "vsx_extract_<mode>"
>    if (element == VECTOR_ELEMENT_SCALAR_64BIT)
>      {
>        if (op0_regno == op1_regno)
>  	return ASM_COMMENT_START " vec_extract to same register";
>  
> -      else if (INT_REGNO_P (op0_regno) && TARGET_DIRECT_MOVE
> +      else if (INT_REGNO_P (op0_regno) && TARGET_POWER8
>  	       && TARGET_POWERPC64)

That fits on one line now.

Thanks,


Segher

Re: [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2)

[will schmidt]

On Mon, 2022-10-17 at 13:08 -0500, Segher Boessenkool wrote:
> On Mon, Sep 19, 2022 at 11:13:20AM -0500, will schmidt wrote:
> >   The _ARCH_PWR8 define is conditional on TARGET_DIRECT_MOVE,
> > and can be disabled by dependent options when it should not be.
> > This manifests in the issue seen in PR101865 where -mno-vsx
> > mistakenly disables _ARCH_PWR8.
> > This change replaces the relevant TARGET_DIRECT_MOVE references
> > with a TARGET_POWER8 entry so that the direct_move and power8
> > features can be enabled or disabled independently.
> 
> We should get rid of TARGET_DIRECT_MOVE altogether.  Please see
> 57f108f5a1e1:
>     rs6000: Disable -m[no-]direct-move (PR85293)
> 
>     The -mno-direct-move option causes a lot of problems, since it
> forces
>     us to be able to generate code for p8 and up with some crucial
>     instructions missing.  This patch removes the -m[no-]direct-move
>     options so that the user cannot put us into this unexpected
> situation
>     anymore.  Internally we still have all the same flags, and they
> are
>     automatically set based on -mcpu; getting rid of that is a lot
> more
>     work and will have to wait for GCC 9 (in some places the flag is
> used
>     to see if we are compiling for a p8 _at all_).
> 
> It did not happen in GCC 9 obviously.  Do you want to take a
> shot?  It
> doesn't have to be all at once, it's probably best if not even -- as
> I
> wrote in the commit message, the flag always was used to mean
> different
> things.

As long as it's OK to be removed, I'll certainly take a shot at it. 
With that in mind that may simplify things for me here.
I expect that
anything currently guarded by DIRECT_MOVE should instead be guarded by
POWER8.


> 
> > The existing (and rather lengthy) commentary for DIRECT_MOVE
> > remains
> > in place in rs6000-c.cc:rs6000_target_modify_macros().  The
> > if-defined logic there will now set a __DIRECT_MOVE__ define when
> > TARGET_DIRECT_MOVE is set, this serves as a placeholder for debug
> > purposes, but is otherwise unused.  This can be removed in a
> > subsequent patch, or in an update of this patch, depending on
> > feedback.
> 
> There should be no such macro, for the same reason there should be no
> -mdirect-move option: it is so very essential to all code we
> generate,
> it *always* is enabled if we have P8 or later.

fair enough.

> 
> > gcc/
> > 	PR Target/101865
> > 	* config/rs6000/rs6000-builtin.cc
> > 	(rs6000_builtin_is_supported): Replace TARGET_DIRECT_MOVE
> > 	usage with TARGET_POWER8.
> 
> Please don't arbitrarily wrap lines.  It is harder to read, and it
> looks
> like something is missing.

> 
> > 	* config/rs6000/rs6000-cpus.def (ISA_2_7_MASKS_SERVER):
> > 	Add OPTION_MASK_POWER8 entry.
> 
> Especially in cases like this, where it looks like you forgot to
> write
> something after the colon.
> 
> > @@ -24046,10 +24045,11 @@ static struct rs6000_opt_mask const
> > rs6000_opt_masks[] =
> >    { "block-ops-vector-pair",	OPTION_MASK_BLOCK_OPS_VECTOR_PA
> > IR,
> >  								false,
> > true  },
> >    { "cmpb",			OPTION_MASK_CMPB,		fal
> > se, true  },
> >    { "crypto",			OPTION_MASK_CRYPTO,		fal
> > se, true  },
> >    { "direct-move",		OPTION_MASK_DIRECT_MOVE,	false,
> > true  },
> > +  { "power8",			OPTION_MASK_POWER8,		fal
> > se, true  },
> 
> Why would we want a #pragma power8 ?

Hmm, thinko on my part, i'll reevaluate.


> 
> > --- a/gcc/config/rs6000/rs6000.opt
> > +++ b/gcc/config/rs6000/rs6000.opt
> > @@ -490,10 +490,15 @@ mcrypto
> >  Target Mask(CRYPTO) Var(rs6000_isa_flags)
> >  Use ISA 2.07 Category:Vector.AES and Category:Vector.SHA2
> > instructions.
> >  
> >  mdirect-move
> >  Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags)
> > WarnRemoved
> > +Enable direct move (ISA 2.07).
> 
> It is undocumented and should remain that, except eventually we
> should
> remove it completely (but leave some stubs so that code in the wild
> keeps compiling).
> 
> > +mpower8
> > +Target Mask(POWER8) Var(rs6000_isa_flags)
> > +Use instructions added in ISA 2.07 (power8).
> 
> There should not be such an option.  It is set by -mcpu=power8 and
> later, but can never be enabled or disabled direfctly by the user.

OK.


Thanks for the detailed review.  :-)
-Will


> 
> > --- a/gcc/config/rs6000/vsx.md
> > +++ b/gcc/config/rs6000/vsx.md
> > @@ -3407,11 +3407,11 @@ (define_insn "vsx_extract_<mode>"
> >    if (element == VECTOR_ELEMENT_SCALAR_64BIT)
> >      {
> >        if (op0_regno == op1_regno)
> >  	return ASM_COMMENT_START " vec_extract to same register";
> >  
> > -      else if (INT_REGNO_P (op0_regno) && TARGET_DIRECT_MOVE
> > +      else if (INT_REGNO_P (op0_regno) && TARGET_POWER8
> >  	       && TARGET_POWERPC64)
> 
> That fits on one line now.
> 
> Thanks,
> 
> 
> Segher

Re: [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2)

[Segher Boessenkool]

Hi!

On Tue, Oct 18, 2022 at 10:17:30AM -0500, will schmidt wrote:
> On Mon, 2022-10-17 at 13:08 -0500, Segher Boessenkool wrote:
> > It did not happen in GCC 9 obviously.  Do you want to take a
> > shot?  It
> > doesn't have to be all at once, it's probably best if not even -- as
> > I
> > wrote in the commit message, the flag always was used to mean
> > different
> > things.
> 
> As long as it's OK to be removed, I'll certainly take a shot at it. 

It is.  Thanks!

> With that in mind that may simplify things for me here.
> I expect that
> anything currently guarded by DIRECT_MOVE should instead be guarded by
> POWER8.

Yes.  Which works just as well for the places that actually check
whether the direct move insns can be used, and for everything else that
wants p8 :-)

> > >    { "direct-move",		OPTION_MASK_DIRECT_MOVE,	false,
> > > true  },
> > > +  { "power8",			OPTION_MASK_POWER8,		fal
> > > se, true  },
> > 
> > Why would we want a #pragma power8 ?
> 
> Hmm, thinko on my part, i'll reevaluate.

The existing "direct-move" is a historical thing, no something to copy
as an example of how things should be done :-)


Segher

Re: [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2)

[Kewen.Lin]

Hi!

on 2022/10/19 00:52, Segher Boessenkool wrote:
> Hi!
> 
> On Tue, Oct 18, 2022 at 10:17:30AM -0500, will schmidt wrote:
>> On Mon, 2022-10-17 at 13:08 -0500, Segher Boessenkool wrote:
>>> It did not happen in GCC 9 obviously.  Do you want to take a
>>> shot?  It
>>> doesn't have to be all at once, it's probably best if not even -- as
>>> I
>>> wrote in the commit message, the flag always was used to mean
>>> different
>>> things.
>>
>> As long as it's OK to be removed, I'll certainly take a shot at it. 
> 
> It is.  Thanks!
> 
>> With that in mind that may simplify things for me here.
>> I expect that
>> anything currently guarded by DIRECT_MOVE should instead be guarded by
>> POWER8.
> 
> Yes.  Which works just as well for the places that actually check
> whether the direct move insns can be used, and for everything else that
> wants p8 :-)
> 

IIUC, this discussion is saying we want to replace all TARGET_DIRECT_MOVE
with TARGET_POWER8?  I may miss something but it sounds wrong to me.
Currently TARGET_DIRECT_MOVE is used to guard many places which rely on
vector (VSR) support.  One example is one place quoted from [1]:

> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
> index e226a93bbe55..be4fb902049d 100644
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -3407,11 +3407,11 @@ (define_insn "vsx_extract_<mode>"
>    if (element == VECTOR_ELEMENT_SCALAR_64BIT)
>      {
>        if (op0_regno == op1_regno)
>  	return ASM_COMMENT_START " vec_extract to same register";
>  
> -      else if (INT_REGNO_P (op0_regno) && TARGET_DIRECT_MOVE
> +      else if (INT_REGNO_P (op0_regno) && TARGET_POWER8
>  	       && TARGET_POWERPC64)
>  	return "mfvsrd %0,%x1";

Now we want TARGET_POWER8 still on if users specify -mcpu=power8 -mno-vsx,
if we guard the above with TARGET_POWER8, it would mean we can still
have "mfvsrd" but it shouldn't be available as it relies on VSX support
which gets disabled explicitly.  So for these kinds of uses of
TARGET_DIRECT_MOVE which are for actual "direct move" insns, they should
be guarded with TARGET_P8_VECTOR instead?

[1] https://gcc.gnu.org/pipermail/gcc-patches/2022-October/603724.html

BR,
Kewen

Re: [PATCH, rs6000] Split TARGET_POWER8 from TARGET_DIRECT_MOVE [PR101865] (2/2)

[Peter Bergner]

I'm picking up Will's patches for this bug.  As an FYI, this is the bug where
_ARCH_PWR8 is conditional on TARGET_DIRECT_MOVE which can be disabled with
-mno-vsx which is bad.

I already posted the cleanup patch that the updated patch for this bug will rely
on, that removed the OPTION_MASK_DIRECT_MOVE because it is fully redundant with
OPTION_MASK_P8_VECTOR.  I've also incorporated some of Ke Wen's review comments
on Will's original patch.  I have a couple of comments on your review though...


On 10/17/22 1:08 PM, Segher Boessenkool wrote:
> On Mon, Sep 19, 2022 at 11:13:20AM -0500, will schmidt wrote:
>> @@ -24046,10 +24045,11 @@ static struct rs6000_opt_mask const rs6000_opt_masks[] =
>>    { "block-ops-vector-pair",	OPTION_MASK_BLOCK_OPS_VECTOR_PAIR,
>>  								false, true  },
>>    { "cmpb",			OPTION_MASK_CMPB,		false, true  },
>>    { "crypto",			OPTION_MASK_CRYPTO,		false, true  },
>>    { "direct-move",		OPTION_MASK_DIRECT_MOVE,	false, true  },
>> +  { "power8",			OPTION_MASK_POWER8,		false, true  },
> 
> Why would we want a #pragma power8 ?

Agreed, we don't want that.  We have target attribute cpu=power8 for that.



>> +mpower8
>> +Target Mask(POWER8) Var(rs6000_isa_flags)
>> +Use instructions added in ISA 2.07 (power8).
> 
> There should not be such an option.  It is set by -mcpu=power8 and
> later, but can never be enabled or disabled direfctly by the user.

So we need an OPTION_MASK_POWER8 to be created for use in rs6000_isa_flags, but
the only way I see that we can do that is to create an option in rs6000.opt.
Did I miss that there is another way?  Otherwise, I was thinking of creating a
dummy option that is WarnRemoved from the start ala:

+;; This option exists only for its MASK.  It is not intended for users.
+mpower8
+Target Mask(POWER8) Var(rs6000_isa_flags) WarnRemoved
+

Is there a better way?  The problem is P8 created lots of new instructions, but
they were basically all vector and htm instructions.  There were no general
GPR or FPR instructions (ie, what we'd think of as base architecture) added,
so there's no other OPTION_MASK_*/TARGET_* we can use as a P8 base architecture
test.

I'll note I tried just a bare "Target Mask(POWER8) Var(rs6000_isa_flags)" with no
option name mentioned at all, but that didn't work, as no OPTION_MASK_POWER8 was
created.

Peter