* [PATCH, rs6000] Follow-on fix for PR target/80210: ICE in extract_insn
@ 2017-09-29 2:26 Peter Bergner
2017-09-29 22:31 ` Segher Boessenkool
0 siblings, 1 reply; 5+ messages in thread
From: Peter Bergner @ 2017-09-29 2:26 UTC (permalink / raw)
To: GCC Patches; +Cc: Segher Boessenkool, Bill Schmidt
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" } } */
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, rs6000] Follow-on fix for PR target/80210: ICE in extract_insn
2017-09-29 2:26 [PATCH, rs6000] Follow-on fix for PR target/80210: ICE in extract_insn Peter Bergner
@ 2017-09-29 22:31 ` Segher Boessenkool
2017-10-02 17:01 ` Peter Bergner
0 siblings, 1 reply; 5+ messages in thread
From: Segher Boessenkool @ 2017-09-29 22:31 UTC (permalink / raw)
To: Peter Bergner; +Cc: GCC Patches, Bill Schmidt
Hi Peter,
On Thu, Sep 28, 2017 at 09:26:00PM -0500, Peter Bergner wrote:
> 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.
[ .. snip ... ]
This all seems correct (but it is so complex that it is hard to be sure;
well it is _less_ complex now!)
> + /* PowerPC 64-bit LE requires at least ISA 2.07. */
> + const char *default_cpu = ((!TARGET_POWERPC64)
> + ? "powerpc"
> + : ((BYTES_BIG_ENDIAN)
> + ? "powerpc64"
> + : "powerpc64le"));
Please remove the parens around !TARGET_POWERPC64 and BYTES_BIG_ENDIAN
while you're at it (one of the copies you removed had that already :-) )
Looks great to me, please commit. But hold off until Monday please, it
will interfere with testing otherwise.
Thanks!
Segher
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, rs6000] Follow-on fix for PR target/80210: ICE in extract_insn
2017-09-29 22:31 ` Segher Boessenkool
@ 2017-10-02 17:01 ` Peter Bergner
2017-10-02 17:16 ` Segher Boessenkool
0 siblings, 1 reply; 5+ messages in thread
From: Peter Bergner @ 2017-10-02 17:01 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: GCC Patches, Bill Schmidt
On 9/29/17 5:31 PM, Segher Boessenkool wrote:
>> + /* PowerPC 64-bit LE requires at least ISA 2.07. */
>> + const char *default_cpu = ((!TARGET_POWERPC64)
>> + ? "powerpc"
>> + : ((BYTES_BIG_ENDIAN)
>> + ? "powerpc64"
>> + : "powerpc64le"));
>
> Please remove the parens around !TARGET_POWERPC64 and BYTES_BIG_ENDIAN
> while you're at it (one of the copies you removed had that already :-) )
Done.
> Looks great to me, please commit. But hold off until Monday please, it
> will interfere with testing otherwise.
Ok, committed now (Monday). I'd also like to back port this to the
GCC 7 and 6 release branches, where the earlier fix was also back
ported to. Ok there after a week or so of burn in on trunk?
Thanks.
Peter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, rs6000] Follow-on fix for PR target/80210: ICE in extract_insn
2017-10-02 17:01 ` Peter Bergner
@ 2017-10-02 17:16 ` Segher Boessenkool
2017-12-15 3:55 ` Peter Bergner
0 siblings, 1 reply; 5+ messages in thread
From: Segher Boessenkool @ 2017-10-02 17:16 UTC (permalink / raw)
To: Peter Bergner; +Cc: GCC Patches, Bill Schmidt
On Mon, Oct 02, 2017 at 12:00:55PM -0500, Peter Bergner wrote:
> On 9/29/17 5:31 PM, Segher Boessenkool wrote:
> >> + /* PowerPC 64-bit LE requires at least ISA 2.07. */
> >> + const char *default_cpu = ((!TARGET_POWERPC64)
> >> + ? "powerpc"
> >> + : ((BYTES_BIG_ENDIAN)
> >> + ? "powerpc64"
> >> + : "powerpc64le"));
> >
> > Please remove the parens around !TARGET_POWERPC64 and BYTES_BIG_ENDIAN
> > while you're at it (one of the copies you removed had that already :-) )
>
> Done.
>
>
> > Looks great to me, please commit. But hold off until Monday please, it
> > will interfere with testing otherwise.
>
> Ok, committed now (Monday). I'd also like to back port this to the
> GCC 7 and 6 release branches, where the earlier fix was also back
> ported to. Ok there after a week or so of burn in on trunk?
Certainly. Thanks!
Segher
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH, rs6000] Follow-on fix for PR target/80210: ICE in extract_insn
2017-10-02 17:16 ` Segher Boessenkool
@ 2017-12-15 3:55 ` Peter Bergner
0 siblings, 0 replies; 5+ messages in thread
From: Peter Bergner @ 2017-12-15 3:55 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: GCC Patches, Bill Schmidt
On 10/2/17 12:10 PM, Segher Boessenkool wrote:
> On Mon, Oct 02, 2017 at 12:00:55PM -0500, Peter Bergner wrote:
>> On 9/29/17 5:31 PM, Segher Boessenkool wrote:
>>> Looks great to me, please commit. But hold off until Monday please, it
>>> will interfere with testing otherwise.
>>
>> Ok, committed now (Monday). I'd also like to back port this to the
>> GCC 7 and 6 release branches, where the earlier fix was also back
>> ported to. Ok there after a week or so of burn in on trunk?
>
> Certainly. Thanks!
It seems I forgot to back port this fix, so it's had a lot of burn in
time! :-) I re-regtested the patch on powerpc64le-linux, powerpc64-linux
and powerpc-linux on both release branches and there were no regressions,
so I went ahead and committed it. Thanks.
Peter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-12-15 3:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-29 2:26 [PATCH, rs6000] Follow-on fix for PR target/80210: ICE in extract_insn Peter Bergner
2017-09-29 22:31 ` Segher Boessenkool
2017-10-02 17:01 ` Peter Bergner
2017-10-02 17:16 ` Segher Boessenkool
2017-12-15 3:55 ` 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).