diff mbox

, Fix PR 80098, Add better checking for disabling features

Message ID 20170412203051.GA28156@ibm-tiger.the-meissners.org
State New
Headers show

Commit Message

Michael Meissner April 12, 2017, 8:30 p.m. UTC 
On Tue, Apr 11, 2017 at 06:04:33PM -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Tue, Apr 11, 2017 at 05:32:41PM -0400, Michael Meissner wrote:
> > PR 80098 is an interaction between -mmodulo (ISA 3.0/power9 GPR modulo
> > instructions) and -mno-vsx where the -mmodulo option enables some of the ISA
> > 3.0 vector features, even though -mno-vsx was specified.
> > 
> > To do this, I added a table for disabling other vector options when the major
> > vector options (-mvsx, -mpower8-vector, and -mpower9-vector) are disabled.
> > 
> > We could extend this if desired for other options (for example, -mno-popcntd
> > could disable -mmodulo and perhaps the vector options).
> 
> Or we could just remove -mmodulo etc.  What good do they do?  They make
> testing infeasible: an exponential number of combinations to test.

We can't remove -mmodulo, -mpopcntd, -mcmpb, -mpopcntb as these are the basic
markers for -mcpu=power9/power7/power6/power5, and lots of other things depend
on these options.  I'm not sure we have a marker for power8 that isn't vector
related.

Yeah, it would be better to have specific ISA levels, but we don't currently
have them.


> > @@ -3967,7 +3969,7 @@ rs6000_option_override_internal (bool gl
> >  #endif
> >  #ifdef OS_MISSING_ALTIVEC
> >    if (OS_MISSING_ALTIVEC)
> > -    set_masks &= ~(OPTION_MASK_ALTIVEC | OPTION_MASK_VSX);
> > +    set_masks &= ~NO_ALTIVEC_MASKS;
> 
> NO_ALTIVEC_MASKS isn't defined anywhere.  Did you send the wrong patch?

I originally had NO_ALTIVEC_MASKS and then I redid the code, naming them
OTHER_<xxx>_VECTOR_MASKS.  I missed fixing the section that disables Altivec
and VSX instructions if the assembler doesn't have Altivec support.

The following patch fixes this.  I reran the tests on a little endian power8
system.

I also tested the code on a big endian power7 machine by building a compiler
and then rebuilding rs6000.o, adding the OS_MISSING_ALTIVEC define.  I verified
by hand that it disables Altivec and VSX support by default.

Can I apply this to the trunk?

[gcc]
2017-04-12  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/80098
	* config/rs6000/rs6000-cpus.def (OTHER_P9_VECTOR_MASKS): Define
	masks of options that should be turned off if the VSX vector
	options are turned off.
	(OTHER_P8_VECTOR_MASKS): Likewise.
	(OTHER_VSX_VECTOR_MASKS): Likewise.
	* config/rs6000/rs6000.c (rs6000_option_override_internal): Call
	rs6000_disable_incompatible_switches to validate no type switches
	like -mvsx.
	(rs6000_incompatible_switch): New function to disallow turning on
	other vector options if -mno-vsx, -mno-power8-vector, or
	-mno-power9-vector are specified.

[gcc/testsuite]
2017-04-12  Michael Meissner  <meissner@linux.vnet.ibm.com>

	PR target/80098
	* gcc.target/powerpc/pr80098-1.c: New test.
	* gcc.target/powerpc/pr80098-2.c: Likewise.
	* gcc.target/powerpc/pr80098-3.c: Likewise.
	* gcc.target/powerpc/pr80098-4.c: Likewise.

Comments

Segher Boessenkool April 14, 2017, 8:41 a.m. UTC | #1 
On Wed, Apr 12, 2017 at 04:30:51PM -0400, Michael Meissner wrote:
> > Or we could just remove -mmodulo etc.  What good do they do?  They make
> > testing infeasible: an exponential number of combinations to test.
> 
> We can't remove -mmodulo, -mpopcntd, -mcmpb, -mpopcntb as these are the basic
> markers for -mcpu=power9/power7/power6/power5, and lots of other things depend
> on these options.

Yes, and that's exactly backward.

The main point though is that we allow fine-grained selection of trivial
ISA additions, which results in an exponential number of possibilities.
But whether some machine insn is generated also matters for *other*
patterns (and elsewhere even), so we really do need to test all possible
combinations; but that cannot be done.  And then bug reports come in,
and we spend more time making patches to the option maze than we spend
on anything else :-/

> I'm not sure we have a marker for power8 that isn't vector related.

I can't think of any.  Pretty much anything in Power8 is vector.


> 2017-04-12  Michael Meissner  <meissner@linux.vnet.ibm.com>
> 
> 	PR target/80098
> 	* config/rs6000/rs6000-cpus.def (OTHER_P9_VECTOR_MASKS): Define
> 	masks of options that should be turned off if the VSX vector
> 	options are turned off.
> 	(OTHER_P8_VECTOR_MASKS): Likewise.
> 	(OTHER_VSX_VECTOR_MASKS): Likewise.
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal): Call
> 	rs6000_disable_incompatible_switches to validate no type switches
> 	like -mvsx.
> 	(rs6000_incompatible_switch): New function to disallow turning on
> 	other vector options if -mno-vsx, -mno-power8-vector, or
> 	-mno-power9-vector are specified.
> 
> [gcc/testsuite]
> 2017-04-12  Michael Meissner  <meissner@linux.vnet.ibm.com>
> 
> 	PR target/80098
> 	* gcc.target/powerpc/pr80098-1.c: New test.
> 	* gcc.target/powerpc/pr80098-2.c: Likewise.
> 	* gcc.target/powerpc/pr80098-3.c: Likewise.
> 	* gcc.target/powerpc/pr80098-4.c: Likewise.


> +/* Handle explicit -mno-vsx, -mno-power8-vector, and -mno-power9-vector options
> +   and turn off all setting other options by default if those options that
> +   depend on the flags that are turned off.  */

Could you this a bit clearer please?

Okay for trunk.  Thanks,


Segher
diff mbox

Patch

Index: gcc/config/rs6000/rs6000-cpus.def
===================================================================
--- gcc/config/rs6000/rs6000-cpus.def	(revision 246826)
+++ gcc/config/rs6000/rs6000-cpus.def	(working copy)
@@ -84,6 +84,30 @@ 
 				 | OPTION_MASK_UPPER_REGS_SF		\
 				 | OPTION_MASK_VSX_SMALL_INTEGER)
 
+/* Flags that need to be turned off if -mno-power9-vector.  */
+#define OTHER_P9_VECTOR_MASKS	(OPTION_MASK_FLOAT128_HW		\
+				 | OPTION_MASK_P9_DFORM_SCALAR		\
+				 | OPTION_MASK_P9_DFORM_VECTOR		\
+				 | OPTION_MASK_P9_MINMAX)
+
+/* Flags that need to be turned off if -mno-power8-vector.  */
+#define OTHER_P8_VECTOR_MASKS	(OTHER_P9_VECTOR_MASKS			\
+				 | OPTION_MASK_P9_VECTOR		\
+				 | OPTION_MASK_DIRECT_MOVE		\
+				 | OPTION_MASK_CRYPTO			\
+				 | OPTION_MASK_UPPER_REGS_SF)		\
+
+/* Flags that need to be turned off if -mno-vsx.  */
+#define OTHER_VSX_VECTOR_MASKS	(OTHER_P8_VECTOR_MASKS			\
+				 | OPTION_MASK_EFFICIENT_UNALIGNED_VSX	\
+				 | OPTION_MASK_FLOAT128_KEYWORD		\
+				 | OPTION_MASK_FLOAT128_TYPE		\
+				 | OPTION_MASK_P8_VECTOR		\
+				 | OPTION_MASK_UPPER_REGS_DI		\
+				 | OPTION_MASK_UPPER_REGS_DF		\
+				 | OPTION_MASK_VSX_SMALL_INTEGER	\
+				 | OPTION_MASK_VSX_TIMODE)
+
 #define POWERPC_7400_MASK	(OPTION_MASK_PPC_GFXOPT | OPTION_MASK_ALTIVEC)
 
 /* Deal with ports that do not have -mstrict-align.  */
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 246826)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -1335,6 +1335,7 @@  static void rs6000_print_isa_options (FI
 				      HOST_WIDE_INT);
 static void rs6000_print_builtin_options (FILE *, int, const char *,
 					  HOST_WIDE_INT);
+static HOST_WIDE_INT rs6000_disable_incompatible_switches (void);
 
 static enum rs6000_reg_type register_to_reg_type (rtx, bool *);
 static bool rs6000_secondary_reload_move (enum rs6000_reg_type,
@@ -3902,6 +3903,7 @@  rs6000_option_override_internal (bool gl
   const char *implicit_cpu = OPTION_TARGET_CPU_DEFAULT;
 
   HOST_WIDE_INT set_masks;
+  HOST_WIDE_INT ignore_masks;
   int cpu_index;
   int tune_index;
   struct cl_target_option *main_target_opt
@@ -3967,7 +3969,8 @@  rs6000_option_override_internal (bool gl
 #endif
 #ifdef OS_MISSING_ALTIVEC
   if (OS_MISSING_ALTIVEC)
-    set_masks &= ~(OPTION_MASK_ALTIVEC | OPTION_MASK_VSX);
+    set_masks &= ~(OPTION_MASK_ALTIVEC | OPTION_MASK_VSX
+		   | OTHER_VSX_VECTOR_MASK);
 #endif
 
   /* Don't override by the processor default if given explicitly.  */
@@ -4270,11 +4273,15 @@  rs6000_option_override_internal (bool gl
   if (TARGET_DEBUG_REG || TARGET_DEBUG_TARGET)
     rs6000_print_isa_options (stderr, 0, "before defaults", rs6000_isa_flags);
 
+  /* Handle explicit -mno-{altivec,vsx,power8-vector,power9-vector} and turn
+     off all of the options that depend on those flags.  */
+  ignore_masks = rs6000_disable_incompatible_switches ();
+
   /* For the newer switches (vsx, dfp, etc.) set some of the older options,
      unless the user explicitly used the -mno-<option> to disable the code.  */
   if (TARGET_P9_VECTOR || TARGET_MODULO || TARGET_P9_DFORM_SCALAR
       || TARGET_P9_DFORM_VECTOR || TARGET_P9_DFORM_BOTH > 0)
-    rs6000_isa_flags |= (ISA_3_0_MASKS_SERVER & ~rs6000_isa_flags_explicit);
+    rs6000_isa_flags |= (ISA_3_0_MASKS_SERVER & ~ignore_masks);
   else if (TARGET_P9_MINMAX)
     {
       if (have_cpu)
@@ -4283,15 +4290,14 @@  rs6000_option_override_internal (bool gl
 	    {
 	      /* legacy behavior: allow -mcpu-power9 with certain
 		 capabilities explicitly disabled.  */
-	      rs6000_isa_flags |=
-		(ISA_3_0_MASKS_SERVER & ~rs6000_isa_flags_explicit);
+	      rs6000_isa_flags |= (ISA_3_0_MASKS_SERVER & ~ignore_masks);
 	      /* However, reject this automatic fix if certain
 		 capabilities required for TARGET_P9_MINMAX support
 		 have been explicitly disabled.  */
 	      if (((OPTION_MASK_VSX | OPTION_MASK_UPPER_REGS_SF
 		    | OPTION_MASK_UPPER_REGS_DF) & rs6000_isa_flags)
 		  != (OPTION_MASK_VSX | OPTION_MASK_UPPER_REGS_SF
-		       | OPTION_MASK_UPPER_REGS_DF))
+		      | OPTION_MASK_UPPER_REGS_DF))
 		error ("-mpower9-minmax incompatible with explicitly disabled options");
 		}
 	  else
@@ -4308,21 +4314,21 @@  rs6000_option_override_internal (bool gl
 	rs6000_isa_flags |= ISA_3_0_MASKS_SERVER;
     }
   else if (TARGET_P8_VECTOR || TARGET_DIRECT_MOVE || TARGET_CRYPTO)
-    rs6000_isa_flags |= (ISA_2_7_MASKS_SERVER & ~rs6000_isa_flags_explicit);
+    rs6000_isa_flags |= (ISA_2_7_MASKS_SERVER & ~ignore_masks);
   else if (TARGET_VSX)
-    rs6000_isa_flags |= (ISA_2_6_MASKS_SERVER & ~rs6000_isa_flags_explicit);
+    rs6000_isa_flags |= (ISA_2_6_MASKS_SERVER & ~ignore_masks);
   else if (TARGET_POPCNTD)
-    rs6000_isa_flags |= (ISA_2_6_MASKS_EMBEDDED & ~rs6000_isa_flags_explicit);
+    rs6000_isa_flags |= (ISA_2_6_MASKS_EMBEDDED & ~ignore_masks);
   else if (TARGET_DFP)
-    rs6000_isa_flags |= (ISA_2_5_MASKS_SERVER & ~rs6000_isa_flags_explicit);
+    rs6000_isa_flags |= (ISA_2_5_MASKS_SERVER & ~ignore_masks);
   else if (TARGET_CMPB)
-    rs6000_isa_flags |= (ISA_2_5_MASKS_EMBEDDED & ~rs6000_isa_flags_explicit);
+    rs6000_isa_flags |= (ISA_2_5_MASKS_EMBEDDED & ~ignore_masks);
   else if (TARGET_FPRND)
-    rs6000_isa_flags |= (ISA_2_4_MASKS & ~rs6000_isa_flags_explicit);
+    rs6000_isa_flags |= (ISA_2_4_MASKS & ~ignore_masks);
   else if (TARGET_POPCNTB)
-    rs6000_isa_flags |= (ISA_2_2_MASKS & ~rs6000_isa_flags_explicit);
+    rs6000_isa_flags |= (ISA_2_2_MASKS & ~ignore_masks);
   else if (TARGET_ALTIVEC)
-    rs6000_isa_flags |= (OPTION_MASK_PPC_GFXOPT & ~rs6000_isa_flags_explicit);
+    rs6000_isa_flags |= (OPTION_MASK_PPC_GFXOPT & ~ignore_masks);
 
   if (TARGET_CRYPTO && !TARGET_ALTIVEC)
     {
@@ -39716,6 +39722,68 @@  rs6000_print_builtin_options (FILE *file
 				 ARRAY_SIZE (rs6000_builtin_mask_names));
 }
 
+/* Handle explicit -mno-vsx, -mno-power8-vector, and -mno-power9-vector options
+   and turn off all setting other options by default if those options that
+   depend on the flags that are turned off.  */
+
+static HOST_WIDE_INT
+rs6000_disable_incompatible_switches (void)
+{
+  HOST_WIDE_INT ignore_masks = rs6000_isa_flags_explicit;
+  size_t i, j;
+
+  static const struct {
+    const HOST_WIDE_INT no_flag;	/* flag explicitly turned off.  */
+    const HOST_WIDE_INT dep_flags;	/* flags that depend on this option.  */
+    const char *const name;		/* name of the switch.  */
+  } flags[] = {
+    { OPTION_MASK_P9_VECTOR,	OTHER_P9_VECTOR_MASKS,	"power9-vector"	},
+    { OPTION_MASK_P8_VECTOR,	OTHER_P8_VECTOR_MASKS,	"power8-vector"	},
+    { OPTION_MASK_VSX,		OTHER_VSX_VECTOR_MASKS,	"vsx"		},
+  };
+
+  for (i = 0; i < ARRAY_SIZE (flags); i++)
+    {
+      HOST_WIDE_INT no_flag = flags[i].no_flag;
+
+      if ((rs6000_isa_flags & no_flag) == 0
+	  && (rs6000_isa_flags_explicit & no_flag) != 0)
+	{
+	  HOST_WIDE_INT dep_flags = flags[i].dep_flags;
+	  HOST_WIDE_INT set_flags = (rs6000_isa_flags_explicit
+				     & rs6000_isa_flags
+				     & dep_flags);
+
+	  if (set_flags)
+	    {
+	      for (j = 0; j < ARRAY_SIZE (rs6000_opt_masks); j++)
+		if ((set_flags & rs6000_opt_masks[j].mask) != 0)
+		  {
+		    set_flags &= ~rs6000_opt_masks[j].mask;
+		    error ("-mno-%s turns off -m%s",
+			   flags[i].name,
+			   rs6000_opt_masks[j].name);
+		  }
+
+	      gcc_assert (!set_flags);
+	    }
+
+	  rs6000_isa_flags &= ~dep_flags;
+	  ignore_masks |= no_flag | dep_flags;
+	}
+    }
+
+  if (!TARGET_P9_VECTOR
+      && (rs6000_isa_flags_explicit & OPTION_MASK_P9_VECTOR) != 0
+      && TARGET_P9_DFORM_BOTH > 0)
+    {
+      error ("-mno-power9-vector turns off -mpower9-dform");
+      TARGET_P9_DFORM_BOTH = 0;
+    }
+
+  return ignore_masks;
+}
+
 
 /* Hook to determine if one function can safely inline another.  */
 
Index: gcc/testsuite/gcc.target/powerpc/pr80098-3.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr80098-3.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr80098-3.c	(revision 0)
@@ -0,0 +1,9 @@ 
+/* { dg-do compile { target { powerpc64*-*-* } } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */
+/* { dg-require-effective-target powerpc_vsx_ok } */
+/* { dg-options "-mcpu=power7 -mno-vsx -mdirect-move -mcrypto" } */
+
+int i;
+
+/* { dg-error "-mno-vsx turns off -mdirect-move" "PR80098" { target *-*-* } 0 } */
+/* { dg-error "-mno-vsx turns off -mcrypto"      "PR80098" { target *-*-* } 0 } */
Index: gcc/testsuite/gcc.target/powerpc/pr80098-4.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr80098-4.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr80098-4.c	(revision 0)
@@ -0,0 +1,8 @@ 
+/* { dg-do compile { target { powerpc64*-*-* && lp64 } } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power7" } } */
+/* { dg-require-effective-target powerpc_vsx_ok } */
+/* { dg-options "-mcpu=power7 -mno-vsx -mvsx-timode" } */
+
+int i;
+
+/* { dg-error "-mno-vsx turns off -mvsx-timode" "PR80098" { target *-*-* } 0 } */
Index: gcc/testsuite/gcc.target/powerpc/pr80098-1.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr80098-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr80098-1.c	(revision 0)
@@ -0,0 +1,9 @@ 
+/* { dg-do compile { target { powerpc64*-*-* } } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power9" } } */
+/* { dg-require-effective-target powerpc_p9vector_ok } */
+/* { dg-options "-mcpu=power9 -mno-power9-vector -mpower9-minmax -mpower9-dform" } */
+
+int i;
+
+/* { dg-error "-mno-power9-vector turns off -mpower9-minmax" "PR80098" { target *-*-* } 0 } */
+/* { dg-error "-mno-power9-vector turns off -mpower9-dform"  "PR80098" { target *-*-* } 0 } */
Index: gcc/testsuite/gcc.target/powerpc/pr80098-2.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr80098-2.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr80098-2.c	(revision 0)
@@ -0,0 +1,9 @@ 
+/* { dg-do compile { target { powerpc64*-*-* } } } */
+/* { dg-skip-if "do not override -mcpu" { powerpc*-*-* } { "-mcpu=*" } { "-mcpu=power8" } } */
+/* { dg-require-effective-target powerpc_p8vector_ok } */
+/* { dg-options "-mcpu=power8 -mno-power8-vector -mdirect-move -mcrypto" } */
+
+int i;
+
+/* { dg-error "-mno-power8-vector turns off -mdirect-move" "PR80098" { target *-*-* } 0 } */
+/* { dg-error "-mno-power8-vector turns off -mcrypto"      "PR80098" { target *-*-* } 0 } */