[PATCH] rs6000: ICE in unrecognizable insn when using -mpower10

[PATCH] rs6000: ICE in unrecognizable insn when using -mpower10

[Peter Bergner]

We get an ICE when using -mpower10 and a -mcpu= value that is older
than power10.  The -mpower10 option requires -mcpu=power10 or later.
The following patch enforces that.

This passed bootstrap and regtesting with no errors.  Ok for trunk?

GCC 10 does not ICE on this test case, so I am not asking for a backport.

Peter


gcc/
	PR target/95907
	* config/rs6000/rs6000.c (rs6000_option_override_internal): Add check
	to require -mcpu=power10 if using -mpower10.

gcc/testsuite/
	PR target/95907
	* g++.target/powerpc/pr95907.C: New test.

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 6bea544d26a..96241a9d0a3 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -4094,6 +4094,14 @@ rs6000_option_override_internal (bool global_init_p)
       rs6000_isa_flags &= ~OPTION_MASK_FLOAT128_HW;
     }
 
+  /* If the user explicitly uses -mpower10, ensure our ISA flags are
+     compatible with it.  */
+  if (TARGET_POWER10
+      && (rs6000_isa_flags_explicit & OPTION_MASK_POWER10) != 0
+      && (processor_target_table[cpu_index].target_enable
+	  & OPTION_MASK_POWER10) == 0)
+    error ("%qs requires %qs", "-mpower10", "-mcpu=power10");
+
   /* Enable -mprefixed by default on power10 systems.  */
   if (TARGET_POWER10 && (rs6000_isa_flags_explicit & OPTION_MASK_PREFIXED) == 0)
     rs6000_isa_flags |= OPTION_MASK_PREFIXED;
diff --git a/gcc/testsuite/g++.target/powerpc/pr95907.C b/gcc/testsuite/g++.target/powerpc/pr95907.C
new file mode 100644
index 00000000000..45d276f25a7
--- /dev/null
+++ b/gcc/testsuite/g++.target/powerpc/pr95907.C
@@ -0,0 +1,7 @@
+// PR target/95907
+// { dg-do compile }
+// { dg-require-effective-target power10_ok }
+// { dg-options "-mpower10" }
+
+int foobool_argc;
+bool foobool() { return foobool_argc; }

Re: [PATCH] rs6000: ICE in unrecognizable insn when using -mpower10

[Segher Boessenkool]

On Thu, Jul 23, 2020 at 08:15:42PM -0500, Peter Bergner wrote:
> We get an ICE when using -mpower10 and a -mcpu= value that is older
> than power10.  The -mpower10 option requires -mcpu=power10 or later.
> The following patch enforces that.

Hi!

> +  /* If the user explicitly uses -mpower10, ensure our ISA flags are
> +     compatible with it.  */
> +  if (TARGET_POWER10
> +      && (rs6000_isa_flags_explicit & OPTION_MASK_POWER10) != 0
> +      && (processor_target_table[cpu_index].target_enable
> +	  & OPTION_MASK_POWER10) == 0)
> +    error ("%qs requires %qs", "-mpower10", "-mcpu=power10");

This still allows -mpower10 without corresponding -mcpu=.  We should
just remove this command like flag (but keep the internal flag); for
power10 we can do that without any issues, it is new (some testcases
will need fixing, but it is that: fixing).


Segher

Re: [PATCH] rs6000: ICE in unrecognizable insn when using -mpower10

[Peter Bergner]

On 7/24/20 6:32 AM, Segher Boessenkool wrote:
> On Thu, Jul 23, 2020 at 08:15:42PM -0500, Peter Bergner wrote:
>> +  /* If the user explicitly uses -mpower10, ensure our ISA flags are
>> +     compatible with it.  */
>> +  if (TARGET_POWER10
>> +      && (rs6000_isa_flags_explicit & OPTION_MASK_POWER10) != 0
>> +      && (processor_target_table[cpu_index].target_enable
>> +	  & OPTION_MASK_POWER10) == 0)
>> +    error ("%qs requires %qs", "-mpower10", "-mcpu=power10");
> 
> This still allows -mpower10 without corresponding -mcpu=.  We should
> just remove this command like flag (but keep the internal flag); for
> power10 we can do that without any issues, it is new (some testcases
> will need fixing, but it is that: fixing).

I think our gcc driver will always pass a -mcpu= option to the compiler.
That said, I too would prefer not even having the option.  However, I
don't know how to remove the -mpower10 option but keep the flag.
You had mentioned -mdirect-move  as one that had bee removed, but
you actually only get a warning if you try and use that option, not
an error, so it hasn't actually been removed.

Peter

Re: [PATCH] rs6000: ICE in unrecognizable insn when using -mpower10

[Segher Boessenkool]

On Fri, Jul 24, 2020 at 11:10:29AM -0500, Peter Bergner wrote:
> On 7/24/20 6:32 AM, Segher Boessenkool wrote:
> > On Thu, Jul 23, 2020 at 08:15:42PM -0500, Peter Bergner wrote:
> >> +  /* If the user explicitly uses -mpower10, ensure our ISA flags are
> >> +     compatible with it.  */
> >> +  if (TARGET_POWER10
> >> +      && (rs6000_isa_flags_explicit & OPTION_MASK_POWER10) != 0
> >> +      && (processor_target_table[cpu_index].target_enable
> >> +	  & OPTION_MASK_POWER10) == 0)
> >> +    error ("%qs requires %qs", "-mpower10", "-mcpu=power10");
> > 
> > This still allows -mpower10 without corresponding -mcpu=.  We should
> > just remove this command like flag (but keep the internal flag); for
> > power10 we can do that without any issues, it is new (some testcases
> > will need fixing, but it is that: fixing).
> 
> I think our gcc driver will always pass a -mcpu= option to the compiler.
> That said, I too would prefer not even having the option.  However, I
> don't know how to remove the -mpower10 option but keep the flag.
> You had mentioned -mdirect-move  as one that had bee removed, but
> you actually only get a warning if you try and use that option, not
> an error, so it hasn't actually been removed.

It *was* removed, until a change in the option infrastructure removed
that feature :-(

It used "Ignore", so that the command line option didn't do anything,
but we still had the flag.  It was

mdirect-move
Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) Ignore Warn(%qs is deprecated)

but after various changes that destroyed this functionality it was
changed to

mdirect-move
Target Undocumented Mask(DIRECT_MOVE) Var(rs6000_isa_flags) WarnRemoved

which seems to actually work, but is completely ununderstandable syntax.


Eventually we want to not use command-line flags to be able to select
this at all, but to do that we will need to change all our current logic
that uses target_flags.  Which isn't so bad, but it will take time.  Not
made easier by that we already *have* split off an rs6000_isa_flags
variable.  It's a mess :-(


Segher