public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH, rs6000] Fix PR target/80210: ICE in extract_insn
@ 2017-08-18  6:39 Peter Bergner
  2017-08-19  0:04 ` Segher Boessenkool
  2017-09-08 19:11 ` Andreas Schwab
  0 siblings, 2 replies; 8+ messages in thread
From: Peter Bergner @ 2017-08-18  6:39 UTC (permalink / raw)
  To: GCC Patches; +Cc: Segher Boessenkool, Bill Schmidt, Michael Meissner

PR target/80210 exposes a problem in rs6000_set_current_function() where
is fails to correctly clear the rs6000_previous_fndecl cache correctly.
With the test case, we notice that rs6000_previous_fndecl is set (when it
shouldn't be) and we end up restoring options from it.  In this case,
we end up disabling HW sqrt (because of the pragma) when we earlier
decided we could generate HW sqrts which leads to an ICE.

The current code in rs6000_set_current_function() is kind of a rats nest,
so I threw it out and rewrote it, modeling it after how S390 and i386
handle it, which correctly clears the *_previous_fndecl caches.
It also makes the code much more readable in my view.

This passed bootstrap and regtesting with no regressions and it fixes
the ICE.  Ok for trunk?

This is also broken in GCC 7, GCC 6 and GCC 5.  Ok for those after this
has been on trunk for a little while and assuming testing passes?

Peter


gcc/
	* config/rs6000/rs6000.c (rs6000_activate_target_options): New function.
	(rs6000_set_current_function): Rewrite function to use it.

gcc/testsuite/

	* gcc.target/powerpc/pr80210.c: New test.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 251171)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -36664,23 +36664,30 @@ rs6000_pragma_target_parse (tree args, t
 /* Remember the last target of rs6000_set_current_function.  */
 static GTY(()) tree rs6000_previous_fndecl;
 
+/* Restore target's globals from NEW_TREE and invalidate the
+   rs6000_previous_fndecl cache.  */
+
+static void
+rs6000_activate_target_options (tree new_tree)
+{
+  cl_target_option_restore (&global_options, TREE_TARGET_OPTION (new_tree));
+  if (TREE_TARGET_GLOBALS (new_tree))
+    restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
+  else if (new_tree == target_option_default_node)
+    restore_target_globals (&default_target_globals);
+  else
+    TREE_TARGET_GLOBALS (new_tree) = save_target_globals_default_opts ();
+  rs6000_previous_fndecl = NULL_TREE;
+}
+
 /* Establish appropriate back-end context for processing the function
    FNDECL.  The argument might be NULL to indicate processing at top
    level, outside of any function scope.  */
 static void
 rs6000_set_current_function (tree fndecl)
 {
-  tree old_tree = (rs6000_previous_fndecl
-		   ? DECL_FUNCTION_SPECIFIC_TARGET (rs6000_previous_fndecl)
-		   : NULL_TREE);
-
-  tree new_tree = (fndecl
-		   ? DECL_FUNCTION_SPECIFIC_TARGET (fndecl)
-		   : NULL_TREE);
-
   if (TARGET_DEBUG_TARGET)
     {
-      bool print_final = false;
       fprintf (stderr, "\n==================== rs6000_set_current_function");
 
       if (fndecl)
@@ -36693,58 +36700,60 @@ rs6000_set_current_function (tree fndecl
 	fprintf (stderr, ", prev_fndecl (%p)", (void *)rs6000_previous_fndecl);
 
       fprintf (stderr, "\n");
+    }
+
+  /* Only change the context if the function changes.  This hook is called
+     several times in the course of compiling a function, and we don't want to
+     slow things down too much or call target_reinit when it isn't safe.  */
+  if (fndecl == rs6000_previous_fndecl)
+    return;
+
+  tree old_tree;
+  if (rs6000_previous_fndecl == NULL_TREE)
+    old_tree = target_option_current_node;
+  else if (DECL_FUNCTION_SPECIFIC_TARGET (rs6000_previous_fndecl))
+    old_tree = DECL_FUNCTION_SPECIFIC_TARGET (rs6000_previous_fndecl);
+  else
+    old_tree = target_option_default_node;
+
+  tree new_tree;
+  if (fndecl == NULL_TREE)
+    {
+      if (old_tree != target_option_current_node)
+	new_tree = target_option_current_node;
+      else
+	new_tree = NULL_TREE;
+    }
+  else
+    {
+      new_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
+      if (new_tree == NULL_TREE)
+	new_tree = target_option_default_node;
+    }
+
+  if (TARGET_DEBUG_TARGET)
+    {
       if (new_tree)
 	{
 	  fprintf (stderr, "\nnew fndecl target specific options:\n");
 	  debug_tree (new_tree);
-	  print_final = true;
 	}
 
       if (old_tree)
 	{
 	  fprintf (stderr, "\nold fndecl target specific options:\n");
 	  debug_tree (old_tree);
-	  print_final = true;
 	}
 
-      if (print_final)
+      if (old_tree != NULL_TREE || new_tree != NULL_TREE)
 	fprintf (stderr, "--------------------\n");
     }
 
-  /* Only change the context if the function changes.  This hook is called
-     several times in the course of compiling a function, and we don't want to
-     slow things down too much or call target_reinit when it isn't safe.  */
-  if (fndecl && fndecl != rs6000_previous_fndecl)
-    {
-      rs6000_previous_fndecl = fndecl;
-      if (old_tree == new_tree)
-	;
-
-      else if (new_tree && new_tree != target_option_default_node)
-	{
-	  cl_target_option_restore (&global_options,
-				    TREE_TARGET_OPTION (new_tree));
-	  if (TREE_TARGET_GLOBALS (new_tree))
-	    restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
-	  else
-	    TREE_TARGET_GLOBALS (new_tree)
-	      = save_target_globals_default_opts ();
-	}
+  if (new_tree && old_tree != new_tree)
+    rs6000_activate_target_options (new_tree);
 
-      else if (old_tree && old_tree != target_option_default_node)
-	{
-	  new_tree = target_option_current_node;
-	  cl_target_option_restore (&global_options,
-				    TREE_TARGET_OPTION (new_tree));
-	  if (TREE_TARGET_GLOBALS (new_tree))
-	    restore_target_globals (TREE_TARGET_GLOBALS (new_tree));
-	  else if (new_tree == target_option_default_node)
-	    restore_target_globals (&default_target_globals);
-	  else
-	    TREE_TARGET_GLOBALS (new_tree)
-	      = save_target_globals_default_opts ();
-	}
-    }
+  if (fndecl)
+    rs6000_previous_fndecl = fndecl;
 }
 
 \f
Index: gcc/testsuite/gcc.target/powerpc/pr80210.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr80210.c	(nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/pr80210.c	(working copy)
@@ -0,0 +1,10 @@
+/* Test for ICE arising from GCC target pragma.  */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+double
+foo (double a)
+{
+  return __builtin_sqrt (a);
+}
+#pragma GCC target "no-powerpc-gpopt"

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2017-09-12 18:27 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-18  6:39 [PATCH, rs6000] Fix PR target/80210: ICE in extract_insn Peter Bergner
2017-08-19  0:04 ` Segher Boessenkool
2017-08-20  5:12   ` Peter Bergner
2017-08-23 15:58     ` Peter Bergner
2017-09-08 19:11 ` Andreas Schwab
2017-09-08 22:30   ` Peter Bergner
2017-09-09  8:45     ` Andreas Schwab
2017-09-12 18:27       ` Peter Bergner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).