Re: [PATCH, rs6000] Fix PR target/80210: ICE in extract_insn

[Peter Bergner <bergner at vnet dot ibm dot com>]

On 9/8/17 2:11 PM, Andreas Schwab wrote:
> Executing on host: /daten/gcc/gcc-20170907/Build/gcc/xgcc -B/daten/gcc/gcc-20170907/Build/gcc/ /daten/gcc/gcc-20170907/gcc/testsuite/gcc.target/powerpc/pr80125.c  -m32    -fno-diagnostics-show-caret -fdiagnostics-color=never   -O2 -maltivec -S -o pr80125.s    (timeout = 300)
> 
> FAIL: gcc.target/powerpc/pr80210.c (internal compiler error)
> FAIL: gcc.target/powerpc/pr80210.c (test for excess errors)

David mentioned this to me earlier that he saw the same thing on AIX.
I didn't see this on my BE builds, because my build scripts were
using the --with-cpu=... configure command and using any -mcpu=...
option will work around the bug.  The bug isn't in my patch, since
the ICE existed before my patch, we just didn't have the test case
to notice before.  

The problem is due to a mismatch between TARGET_DEFAULT, which contains
MASK_PPC_GPOPT and the ISA flags for the default "powerpc64" 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 global_init_p arg set to "true" and we basically
set rs6000_isa_flags to TARGET_DEFAULT.  This is because we do not
have a -mcpu= value nor do we have an "implicit_cpu", which forces
us to use TARGET_DEFAULT.  With this, init_all_optabs() thinks we
can generate 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 with
global_init_p arg now 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;
    }

So now we act as if the user explicitly passed in a -mcpu= option, then:

  ...
  /* If we have a cpu, either through an explicit -mcpu=<xxx> or if the
     compiler was configured with --with-cpu=<xxx>, replace all of the ISA bits
     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)
    {
      rs6000_isa_flags &= ~set_masks;
      rs6000_isa_flags |= (processor_target_table[cpu_index].target_enable
                           & set_masks);
    }
  else
    {
      /* If no -mcpu=<xxx>, inherit any default options that were cleared via
         POWERPC_MASKS.  Originally, TARGET_DEFAULT was used to initialize
         target_flags via the TARGET_DEFAULT_TARGET_FLAGS hook.  When we switched
         to using rs6000_isa_flags, we need to do the initialization here.

         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);
      rs6000_isa_flags |= (flags & ~rs6000_isa_flags_explicit);
    }

So the first time through here with global_init_p == true, have_cpu is set to
false and we get TARGET_DEFAULT.  The next time we come here, global_init_p
== false and we set have_cpu to true because main_target_opt is non-NULL and
the cpu_index value is set to "powerpc64" (for -m64 compiles) or "powerpc"
(for -m32 compiles).  This causes us to now grab the ISA flags from:

  processor_target_table[cpu_index].target_enable

instead of from TARGET_DEFAULT and neither "powerpc64" or "powerpc" contain
the MASK_PPC_GPOPT flag, which leads us to ICE because the optabs allows us
to generate the HW sqrt pattern, but our ISA flags don't allow it.

This doesn't affect LE builds, because it has a TARGET_DEFAULT value that matches
the "powerpc64l" default masks.  We also enforce passing a -mcpu=power8 option
when the user doesn't explicitly use one, so again, not a problem.

This also doesn't affect --target=powerpc-linux builds or --target=powerpc64-linux
builds that default to 32-bit binaries, because we use a value of TARGET_DEFAULT == 0
(for both -m32 and -m64), so the first time through rs6000_option_override_internal(),
we end up using processor_target_table[cpu_index].target_enable right from the beginning.

The following patch fixes the problem I saw on Linux and bootstraps and regtests
with no regressions on LE and BE (running testsuite in both 32-bit and 64-bit
modes).  I was waiting to submit this until David had a chance to verify this
fixes the problem on AIX.

Peter



Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 251386)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -3993,34 +3993,19 @@ rs6000_option_override_internal (bool gl
     }
   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;
+      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);
+      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);
 
   if (have_cpu)
     {
 #ifndef HAVE_AS_POWER9
-      if (processor_target_table[rs6000_cpu_index].processor 
-	  == PROCESSOR_POWER9)
+      if (processor_target_table[cpu_index].processor == PROCESSOR_POWER9)
 	{
 	  have_cpu = false;
 	  warning (0, "will not generate power9 instructions because "
@@ -4028,8 +4013,7 @@ rs6000_option_override_internal (bool gl
 	}
 #endif
 #ifndef HAVE_AS_POWER8
-      if (processor_target_table[rs6000_cpu_index].processor
-	  == PROCESSOR_POWER8)
+      if (processor_target_table[cpu_index].processor == PROCESSOR_POWER8)
 	{
 	  have_cpu = false;
 	  warning (0, "will not generate power8 instructions because "
@@ -4037,8 +4021,7 @@ rs6000_option_override_internal (bool gl
 	}
 #endif
 #ifndef HAVE_AS_POPCNTD
-      if (processor_target_table[rs6000_cpu_index].processor
-	  == PROCESSOR_POWER7)
+      if (processor_target_table[cpu_index].processor == PROCESSOR_POWER7)
 	{
 	  have_cpu = false;
 	  warning (0, "will not generate power7 instructions because "
@@ -4046,8 +4029,7 @@ rs6000_option_override_internal (bool gl
 	}
 #endif
 #ifndef HAVE_AS_DFP
-      if (processor_target_table[rs6000_cpu_index].processor
-	  == PROCESSOR_POWER6)
+      if (processor_target_table[cpu_index].processor == PROCESSOR_POWER6)
 	{
 	  have_cpu = false;
 	  warning (0, "will not generate power6 instructions because "
@@ -4055,26 +4037,13 @@ rs6000_option_override_internal (bool gl
 	}
 #endif
 #ifndef HAVE_AS_POPCNTB
-      if (processor_target_table[rs6000_cpu_index].processor
-	  == PROCESSOR_POWER5)
+      if (processor_target_table[cpu_index].processor == PROCESSOR_POWER5)
 	{
 	  have_cpu = false;
 	  warning (0, "will not generate power5 instructions because "
 		   "assembler lacks power5 support");
 	}
 #endif
-
-      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);
-	}
     }
 
   /* If we have a cpu, either through an explicit -mcpu=<xxx> or if the
@@ -4084,6 +4053,7 @@ rs6000_option_override_internal (bool gl
      TARGET_DEFAULT.  */
   if (have_cpu)
     {
+      rs6000_cpu_index = cpu_index;
       rs6000_isa_flags &= ~set_masks;
       rs6000_isa_flags |= (processor_target_table[cpu_index].target_enable
 			   & set_masks);
@@ -4097,8 +4067,20 @@ 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"));
+	  cpu_index = rs6000_cpu_name_lookup (default_cpu);
+	  flags = processor_target_table[cpu_index].target_enable;
+	}
       rs6000_isa_flags |= (flags & ~rs6000_isa_flags_explicit);
     }