This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH, rs6000] Follow-on fix for PR target/80210: ICE in extract_insn


This patch fixes two new issues exposed (but not caused) by the original 
test case added for PR80210 as well as a modified version of that test case.

The first problem is that the test case pr80210.c ICEs on 32-bit compiles
that do not pass either an explicit or implicit -mcpu=...  option.
I did not see this during my testing, because my powerpc-linux builds were
done on a 64-bit system and I built my compiler using the --with-cpu=default32
configure option which hid the ICE.

This problem is due to a mismatch between TARGET_DEFAULT, which contains
MASK_PPC_GPOPT and the ISA flags for the default "powerpc64"/"powerpc" cpu,
which does not contain MASK_PPC_GPOPT and how rs6000_option_override_internal()
decides which one to use.  The failure scenario is, early on we call
init_all_optabs() which setups up a table which describes which patterns
that generate some HW insns are "valid".  Before we call init_all_optabs(),
rs6000_option_override_internal() gets called with the global_init_p arg
set to "true" and we basically set rs6000_isa_flags to TARGET_DEFAULT.
We also execute the following code because we didn't pass in a -mcpu=
option, so rs6000_cpu_index gets set to "powerpc64"/"powerpc"'s index
into the cpu table.

      if (!have_cpu)
        {
          /* PowerPC 64-bit LE requires at least ISA 2.07.  */
          const char *default_cpu = (!TARGET_POWERPC64
                                     ? "powerpc"
                                     : (BYTES_BIG_ENDIAN
                                        ? "powerpc64"
                                        : "powerpc64le"));

          rs6000_cpu_index = cpu_index = rs6000_cpu_name_lookup (default_cpu);
        }

With this, init_all_optabs() thinks we can generate (as it should) a HW sqrt,
so it enables generating its pattern.

Later, after we've scanned the entire file, we go to expand our function
into RTL and we reset our compiler options and we end up calling
rs6000_option_override_internal() again, but this time with the global_init_p
arg now set to false and we encounter this code:

  struct cl_target_option *main_target_opt
    = ((global_init_p || target_option_default_node == NULL)
       ? NULL : TREE_TARGET_OPTION (target_option_default_node));

This ends up setting main_target_opt to a non-NULL value, then:

  ...
  else if (main_target_opt != NULL && main_target_opt->x_rs6000_cpu_index >= 0)
    {
      rs6000_cpu_index = cpu_index = main_target_opt->x_rs6000_cpu_index;
      have_cpu = true;
    }

This causes us to use the saved rs6000_cpu_index value and act as if the
user passed it in, so we restore the rs6000_isa_flags from the saved
default cpu rather than the TARGET_DEFAULT flags.  Since the default
cpus don't include the MASK_PPC_GPOPT flag, we eventually ICE.

This patch fixes the pr80210.c ICE by correctly setting the rs6000_cpu_index
value which in turn makes us use the correct rs6000_isa_flags value.
I also fixed the setting of the rs6000_tune_index value that was also
being set incorrectly sometimes, but of course, it doesn't lead to an
ICE, just wrong scheduling.

The second problem was exposed by compiling the pr80210.c test case, but
with the #pragma moved to the beginning of the file.  In this case, we
should disable the generating of the HW sqrt pattern in the optabs.
The ICE showed that we were still generating the HQ sqrt pattern when
we shouldn't have.  This problem is basically the dual of the other
problem, in that we are not correctly saving and restoring the optab
values.  The problem here is that rs6000_pragma_target_parse () did not
call rs6000_activate_target_options () which ends up resetting the
optabs values associated with the rs6000_isa_flags value.

This passed bootstrap and regtesting on powerpc64le-linux as well as
on powerpc64-linux and running the test suite in both 32-bit and 64-bit
modes.  Ok for trunk?

Peter


gcc/
	PR target/80210
	* config/rs6000/rs6000.c (rs6000_option_override_internal): Rewrite
	function to not use the have_cpu variable.  Do not set cpu_index,
	rs6000_cpu_index or rs6000_tune_index if we end up using TARGET_DEFAULT
	or the default cpu.
	(rs6000_valid_attribute_p): Remove duplicate initializations of
	old_optimize and func_optimize.
	(rs6000_pragma_target_parse): Call rs6000_activate_target_options ().
	(rs6000_activate_target_options): Make global.
	* config/rs6000/rs6000-protos.h (rs6000_activate_target_options): Add
	prototype.

gcc/testsuite/
	PR target/80210
	* gcc.target/powerpc/pr80210-2.c: New test.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 253232)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -3992,14 +3992,10 @@ static bool
 rs6000_option_override_internal (bool global_init_p)
 {
   bool ret = true;
-  bool have_cpu = false;
-
-  /* The default cpu requested at configure time, if any.  */
-  const char *implicit_cpu = OPTION_TARGET_CPU_DEFAULT;
 
   HOST_WIDE_INT set_masks;
   HOST_WIDE_INT ignore_masks;
-  int cpu_index;
+  int cpu_index = -1;
   int tune_index;
   struct cl_target_option *main_target_opt
     = ((global_init_p || target_option_default_node == NULL)
@@ -4078,93 +4074,51 @@ rs6000_option_override_internal (bool gl
      with -mtune on the command line.  Process a '--with-cpu' configuration
      request as an implicit --cpu.  */
   if (rs6000_cpu_index >= 0)
-    {
-      cpu_index = rs6000_cpu_index;
-      have_cpu = true;
-    }
+    cpu_index = rs6000_cpu_index;
   else if (main_target_opt != NULL && main_target_opt->x_rs6000_cpu_index >= 0)
-    {
-      rs6000_cpu_index = cpu_index = main_target_opt->x_rs6000_cpu_index;
-      have_cpu = true;
-    }
-  else if (implicit_cpu)
-    {
-      rs6000_cpu_index = cpu_index = rs6000_cpu_name_lookup (implicit_cpu);
-      have_cpu = true;
-    }
-  else
-    {
-      /* PowerPC 64-bit LE requires at least ISA 2.07.  */
-      const char *default_cpu = ((!TARGET_POWERPC64)
-				 ? "powerpc"
-				 : ((BYTES_BIG_ENDIAN)
-				    ? "powerpc64"
-				    : "powerpc64le"));
-
-      rs6000_cpu_index = cpu_index = rs6000_cpu_name_lookup (default_cpu);
-      have_cpu = false;
-    }
-
-  gcc_assert (cpu_index >= 0);
+    cpu_index = main_target_opt->x_rs6000_cpu_index;
+  else if (OPTION_TARGET_CPU_DEFAULT)
+    cpu_index = rs6000_cpu_name_lookup (OPTION_TARGET_CPU_DEFAULT);
 
-  if (have_cpu)
+  if (cpu_index >= 0)
     {
-#ifndef HAVE_AS_POWER9
-      if (processor_target_table[rs6000_cpu_index].processor 
-	  == PROCESSOR_POWER9)
+      const char *unavailable_cpu = NULL;
+      switch (processor_target_table[cpu_index].processor)
 	{
-	  have_cpu = false;
-	  warning (0, "will not generate power9 instructions because "
-		   "assembler lacks power9 support");
-	}
+#ifndef HAVE_AS_POWER9
+	case PROCESSOR_POWER9:
+	  unavailable_cpu = "power9";
+	  break;
 #endif
 #ifndef HAVE_AS_POWER8
-      if (processor_target_table[rs6000_cpu_index].processor
-	  == PROCESSOR_POWER8)
-	{
-	  have_cpu = false;
-	  warning (0, "will not generate power8 instructions because "
-		   "assembler lacks power8 support");
-	}
+	case PROCESSOR_POWER8:
+	  unavailable_cpu = "power8";
+	  break;
 #endif
 #ifndef HAVE_AS_POPCNTD
-      if (processor_target_table[rs6000_cpu_index].processor
-	  == PROCESSOR_POWER7)
-	{
-	  have_cpu = false;
-	  warning (0, "will not generate power7 instructions because "
-		   "assembler lacks power7 support");
-	}
+	case PROCESSOR_POWER7:
+	  unavailable_cpu = "power7";
+	  break;
 #endif
 #ifndef HAVE_AS_DFP
-      if (processor_target_table[rs6000_cpu_index].processor
-	  == PROCESSOR_POWER6)
-	{
-	  have_cpu = false;
-	  warning (0, "will not generate power6 instructions because "
-		   "assembler lacks power6 support");
-	}
+	case PROCESSOR_POWER6:
+	  unavailable_cpu = "power6";
+	  break;
 #endif
 #ifndef HAVE_AS_POPCNTB
-      if (processor_target_table[rs6000_cpu_index].processor
-	  == PROCESSOR_POWER5)
-	{
-	  have_cpu = false;
-	  warning (0, "will not generate power5 instructions because "
-		   "assembler lacks power5 support");
-	}
+	case PROCESSOR_POWER5:
+	  unavailable_cpu = "power5";
+	  break;
 #endif
-
-      if (!have_cpu)
+	default:
+	  break;
+	}
+      if (unavailable_cpu)
 	{
-	  /* PowerPC 64-bit LE requires at least ISA 2.07.  */
-	  const char *default_cpu = (!TARGET_POWERPC64
-				     ? "powerpc"
-				     : (BYTES_BIG_ENDIAN
-					? "powerpc64"
-					: "powerpc64le"));
-
-	  rs6000_cpu_index = cpu_index = rs6000_cpu_name_lookup (default_cpu);
+	  cpu_index = -1;
+	  warning (0, "will not generate %qs instructions because "
+		   "assembler lacks %qs support", unavailable_cpu,
+		   unavailable_cpu);
 	}
     }
 
@@ -4173,8 +4127,9 @@ rs6000_option_override_internal (bool gl
      with those from the cpu, except for options that were explicitly set.  If
      we don't have a cpu, do not override the target bits set in
      TARGET_DEFAULT.  */
-  if (have_cpu)
+  if (cpu_index >= 0)
     {
+      rs6000_cpu_index = cpu_index;
       rs6000_isa_flags &= ~set_masks;
       rs6000_isa_flags |= (processor_target_table[cpu_index].target_enable
 			   & set_masks);
@@ -4188,14 +4143,26 @@ rs6000_option_override_internal (bool gl
 
 	 If there is a TARGET_DEFAULT, use that.  Otherwise fall back to using
 	 -mcpu=powerpc, -mcpu=powerpc64, or -mcpu=powerpc64le defaults.  */
-      HOST_WIDE_INT flags = ((TARGET_DEFAULT) ? TARGET_DEFAULT
-			     : processor_target_table[cpu_index].target_enable);
+      HOST_WIDE_INT flags;
+      if (TARGET_DEFAULT)
+	flags = TARGET_DEFAULT;
+      else
+	{
+	  /* PowerPC 64-bit LE requires at least ISA 2.07.  */
+	  const char *default_cpu = ((!TARGET_POWERPC64)
+				     ? "powerpc"
+				     : ((BYTES_BIG_ENDIAN)
+					? "powerpc64"
+					: "powerpc64le"));
+	  int default_cpu_index = rs6000_cpu_name_lookup (default_cpu);
+	  flags = processor_target_table[default_cpu_index].target_enable;
+	}
       rs6000_isa_flags |= (flags & ~rs6000_isa_flags_explicit);
     }
 
   if (rs6000_tune_index >= 0)
     tune_index = rs6000_tune_index;
-  else if (have_cpu)
+  else if (cpu_index >= 0)
     rs6000_tune_index = tune_index = cpu_index;
   else
     {
@@ -4207,7 +4174,7 @@ rs6000_option_override_internal (bool gl
       for (i = 0; i < ARRAY_SIZE (processor_target_table); i++)
 	if (processor_target_table[i].processor == tune_proc)
 	  {
-	    rs6000_tune_index = tune_index = i;
+	    tune_index = i;
 	    break;
 	  }
     }
@@ -4334,7 +4301,7 @@ rs6000_option_override_internal (bool gl
     rs6000_isa_flags |= (ISA_3_0_MASKS_SERVER & ~ignore_masks);
   else if (TARGET_P9_MINMAX)
     {
-      if (have_cpu)
+      if (cpu_index >= 0)
 	{
 	  if (cpu_index == PROCESSOR_POWER9)
 	    {
@@ -4837,7 +4804,7 @@ rs6000_option_override_internal (bool gl
 
     default:
 
-      if (have_cpu && !(rs6000_isa_flags_explicit & OPTION_MASK_ISEL))
+      if (cpu_index >= 0 && !(rs6000_isa_flags_explicit & OPTION_MASK_ISEL))
 	rs6000_isa_flags &= ~OPTION_MASK_ISEL;
 
       break;
@@ -36874,9 +36841,9 @@ rs6000_valid_attribute_p (tree fndecl,
 {
   struct cl_target_option cur_target;
   bool ret;
-  tree old_optimize = build_optimization_node (&global_options);
+  tree old_optimize;
   tree new_target, new_optimize;
-  tree func_optimize = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl);
+  tree func_optimize;
 
   gcc_assert ((fndecl != NULL_TREE) && (args != NULL_TREE));
 
@@ -37011,6 +36978,7 @@ rs6000_pragma_target_parse (tree args, t
     }
 
   target_option_current_node = cur_tree;
+  rs6000_activate_target_options (target_option_current_node);
 
   /* If we have the preprocessor linked in (i.e. C or C++ languages), possibly
      change the macros that are defined.  */
@@ -37051,7 +37019,7 @@ static GTY(()) tree rs6000_previous_fnde
 /* Restore target's globals from NEW_TREE and invalidate the
    rs6000_previous_fndecl cache.  */
 
-static void
+void
 rs6000_activate_target_options (tree new_tree)
 {
   cl_target_option_restore (&global_options, TREE_TARGET_OPTION (new_tree));
Index: gcc/config/rs6000/rs6000-protos.h
===================================================================
--- gcc/config/rs6000/rs6000-protos.h	(revision 253232)
+++ gcc/config/rs6000/rs6000-protos.h	(working copy)
@@ -230,6 +230,7 @@ extern void rs6000_cpu_cpp_builtins (str
 #ifdef TREE_CODE
 extern bool rs6000_pragma_target_parse (tree, tree);
 #endif
+extern void rs6000_activate_target_options (tree new_tree);
 extern void rs6000_target_modify_macros (bool, HOST_WIDE_INT, HOST_WIDE_INT);
 extern void (*rs6000_target_modify_macros_ptr) (bool, HOST_WIDE_INT,
 						HOST_WIDE_INT);
Index: gcc/testsuite/gcc.target/powerpc/pr80210-2.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr80210-2.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr80210-2.c	(revision 0)
@@ -0,0 +1,11 @@
+/* Test for ICE arising from GCC target pragma.  */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#pragma GCC target "no-powerpc-gpopt"
+double
+foo (double a)
+{
+  return __builtin_sqrt (a);
+}
+/* { dg-final { scan-assembler-not "fsqrt" } } */


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]