This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][AArch64] PR target/70002: Make aarch64_set_current_function play nice with pragma resetting
- From: Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>
- To: James Greenhalgh <james dot greenhalgh at arm dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Marcus Shawcroft <marcus dot shawcroft at arm dot com>, Richard Earnshaw <Richard dot Earnshaw at arm dot com>
- Date: Fri, 11 Mar 2016 15:27:33 +0000
- Subject: Re: [PATCH][AArch64] PR target/70002: Make aarch64_set_current_function play nice with pragma resetting
- Authentication-results: sourceware.org; auth=none
- References: <56D82223 dot 8020805 at foss dot arm dot com> <20160310145147 dot GD38844 at arm dot com>
On 10/03/16 14:51, James Greenhalgh wrote:
On Thu, Mar 03, 2016 at 11:38:11AM +0000, Kyrill Tkachov wrote:
Hi all,
This patch fixes the ICE that was introduced by my earlier patch to aarch64_set_current_function:
FAIL: gcc.dg/torture/pr52429.c -O2 -flto -fno-use-linker-plugin -flto-partition=none (internal compiler error)
And it also fixes a bug that I was working on separately relating to popping pragmas.
The patch rewrites the aarch64_set_current_function implementation to be the same as the one in the arm port
that Christian wrote and which is simpler than the existing implementation and has been tested for some time
without problems. I've thought this was the way to go but was hoping to do it for GCC 7 instead, but I think
given the ICE we'd rather have consistent implementations of this hook between arm and aarch64 (and ideally
this should be moved into the midend for all targets, I don't see much target-specific information in the
implementation of this across the targets, but not at this stage).
Similar to that implementation the setting and restoring of the target globals is factored into a separate
function that is used in aarch64_set_current_function and the pragma handling function to tell the midend
when to reinitialise its structures.
This patch fixes the ICE, the testcase attached, and passes bootstrap and regression testing on
aarch64-none-linux-gnu.
Sorry for missing the ICE originally.
Ok for trunk?
OK with the typos below fixed.
Thanks, I've committed the attached as r234141.
Kyrill
2016-03-11 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
PR target/70002
* config/aarch64/aarch64-protos.h
(aarch64_save_restore_target_globals): New prototype.
* config/aarch64/aarch64-c.c (aarch64_pragma_target_parse):
Call the above when popping pragma.
* config/aarch64/aarch64.c (aarch64_save_restore_target_globals):
New function.
(aarch64_set_current_function): Rewrite using the above.
2016-03-11 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
PR target/70002
PR target/69245
* gcc.target/aarch64/pr69245_2.c: New test.
diff --git a/gcc/config/aarch64/aarch64-c.c b/gcc/config/aarch64/aarch64-c.c
index 3590ae0daa5d80050b0f81cd6ab9a7779f463516..e057daaec24c0add673d0b2c776d4c4c43d1f0ea 100644
--- a/gcc/config/aarch64/aarch64-c.c
+++ b/gcc/config/aarch64/aarch64-c.c
@@ -178,6 +178,12 @@ aarch64_pragma_target_parse (tree args, tree pop_target)
cpp_opts->warn_unused_macros = saved_warn_unused_macros;
+ /* If we're popping or reseting make sure to update the globals so that
+ the optab availability predicates get recomputed. */
+ if (pop_target)
+ aarch64_save_restore_target_globals (pop_target);
+
+
Extra newline.
/* Initialize SIMD builtins if we haven't already.
Set current_target_pragma to NULL for the duration so that
the builtin initialization code doesn't try to tag the functions
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index e4e49fc9ccc3d568c84b35c1a0c0733475017cca..c40d2b0c78494b50508c1b5135b8ee7676a61631 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -361,6 +361,7 @@ void aarch64_emit_call_insn (rtx);
void aarch64_register_pragmas (void);
void aarch64_relayout_simd_types (void);
void aarch64_reset_previous_fndecl (void);
+void aarch64_save_restore_target_globals (tree);
void aarch64_emit_approx_rsqrt (rtx, rtx);
/* Initialize builtins for SIMD intrinsics. */
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 1e10d9798ddc5f5d2aac4255d3a8fe4ecaf1402a..a05160e08d0474ed9c1e2afa1d00375839417034 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -8570,6 +8570,21 @@ aarch64_reset_previous_fndecl (void)
aarch64_previous_fndecl = NULL;
}
+/* Restore or save the TREE_TARGET_GLOBALS from or to NEW_TREE.
+ Used by aarch64_set_current_function and aarch64_pragma_target_parse to
+ make sure optab availability predicates are recomputed when necessary. */
+
+void
+aarch64_save_restore_target_globals (tree 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 ();
+}
+
/* Implement TARGET_SET_CURRENT_FUNCTION. Unpack the codegen decisions
like tuning and ISA features from the DECL_FUNCTION_SPECIFIC_TARGET
of the function, if such exists. This function may be called multiple
@@ -8579,63 +8594,32 @@ aarch64_reset_previous_fndecl (void)
static void
aarch64_set_current_function (tree fndecl)
{
+ if (!fndecl || fndecl == aarch64_previous_fndecl)
+ return;
+
tree old_tree = (aarch64_previous_fndecl
? DECL_FUNCTION_SPECIFIC_TARGET (aarch64_previous_fndecl)
: NULL_TREE);
- tree new_tree = (fndecl
- ? DECL_FUNCTION_SPECIFIC_TARGET (fndecl)
- : NULL_TREE);
-
+ tree new_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
- if (fndecl && fndecl != aarch64_previous_fndecl)
- {
- aarch64_previous_fndecl = fndecl;
- if (old_tree == new_tree)
- ;
+ /* If current function has no attributes but previous one did,
s/but previous/but the previous/
+ use the default node. */
+ if (!new_tree && old_tree)
+ new_tree = target_option_default_node;
- else if (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
- TREE_TARGET_GLOBALS (new_tree)
- = save_target_globals_default_opts ();
- }
+ /* If nothing to do return. #pragma GCC reset or #pragma GCC pop to
s/to do return/to do, return/
Thanks,
James
diff --git a/gcc/config/aarch64/aarch64-c.c b/gcc/config/aarch64/aarch64-c.c
index 3590ae0daa5d80050b0f81cd6ab9a7779f463516..e64dc7676ccae10e87ade3946904408fe425730d 100644
--- a/gcc/config/aarch64/aarch64-c.c
+++ b/gcc/config/aarch64/aarch64-c.c
@@ -178,6 +178,11 @@ aarch64_pragma_target_parse (tree args, tree pop_target)
cpp_opts->warn_unused_macros = saved_warn_unused_macros;
+ /* If we're popping or reseting make sure to update the globals so that
+ the optab availability predicates get recomputed. */
+ if (pop_target)
+ aarch64_save_restore_target_globals (pop_target);
+
/* Initialize SIMD builtins if we haven't already.
Set current_target_pragma to NULL for the duration so that
the builtin initialization code doesn't try to tag the functions
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index e4e49fc9ccc3d568c84b35c1a0c0733475017cca..c40d2b0c78494b50508c1b5135b8ee7676a61631 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -361,6 +361,7 @@ void aarch64_emit_call_insn (rtx);
void aarch64_register_pragmas (void);
void aarch64_relayout_simd_types (void);
void aarch64_reset_previous_fndecl (void);
+void aarch64_save_restore_target_globals (tree);
void aarch64_emit_approx_rsqrt (rtx, rtx);
/* Initialize builtins for SIMD intrinsics. */
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 1e10d9798ddc5f5d2aac4255d3a8fe4ecaf1402a..cbb4f211f24d882e4f2bb0c512896a5f209e2a89 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -8570,6 +8570,21 @@ aarch64_reset_previous_fndecl (void)
aarch64_previous_fndecl = NULL;
}
+/* Restore or save the TREE_TARGET_GLOBALS from or to NEW_TREE.
+ Used by aarch64_set_current_function and aarch64_pragma_target_parse to
+ make sure optab availability predicates are recomputed when necessary. */
+
+void
+aarch64_save_restore_target_globals (tree 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 ();
+}
+
/* Implement TARGET_SET_CURRENT_FUNCTION. Unpack the codegen decisions
like tuning and ISA features from the DECL_FUNCTION_SPECIFIC_TARGET
of the function, if such exists. This function may be called multiple
@@ -8579,63 +8594,32 @@ aarch64_reset_previous_fndecl (void)
static void
aarch64_set_current_function (tree fndecl)
{
+ if (!fndecl || fndecl == aarch64_previous_fndecl)
+ return;
+
tree old_tree = (aarch64_previous_fndecl
? DECL_FUNCTION_SPECIFIC_TARGET (aarch64_previous_fndecl)
: NULL_TREE);
- tree new_tree = (fndecl
- ? DECL_FUNCTION_SPECIFIC_TARGET (fndecl)
- : NULL_TREE);
-
+ tree new_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
- if (fndecl && fndecl != aarch64_previous_fndecl)
- {
- aarch64_previous_fndecl = fndecl;
- if (old_tree == new_tree)
- ;
+ /* If current function has no attributes but the previous one did,
+ use the default node. */
+ if (!new_tree && old_tree)
+ new_tree = target_option_default_node;
- else if (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
- TREE_TARGET_GLOBALS (new_tree)
- = save_target_globals_default_opts ();
- }
+ /* If nothing to do, return. #pragma GCC reset or #pragma GCC pop to
+ the default have been handled by aarch64_save_restore_target_globals from
+ aarch64_pragma_target_parse. */
+ if (old_tree == new_tree)
+ return;
- 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 ();
- }
- }
+ aarch64_previous_fndecl = fndecl;
- if (!fndecl)
- return;
+ /* First set the target options. */
+ cl_target_option_restore (&global_options, TREE_TARGET_OPTION (new_tree));
- /* If we turned on SIMD make sure that any vector parameters are re-laid out
- so that they use proper vector modes. */
- if (TARGET_SIMD)
- {
- tree parms = DECL_ARGUMENTS (fndecl);
- for (; parms && parms != void_list_node; parms = TREE_CHAIN (parms))
- {
- if (TREE_CODE (parms) == PARM_DECL
- && VECTOR_TYPE_P (TREE_TYPE (parms))
- && DECL_MODE (parms) != TYPE_MODE (TREE_TYPE (parms)))
- relayout_decl (parms);
- }
- }
+ aarch64_save_restore_target_globals (new_tree);
}
/* Enum describing the various ways we can handle attributes.
diff --git a/gcc/testsuite/gcc.target/aarch64/pr69245_2.c b/gcc/testsuite/gcc.target/aarch64/pr69245_2.c
new file mode 100644
index 0000000000000000000000000000000000000000..6743f5d0ccca5ee6716504749255b0fdbe633a8f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr69245_2.c
@@ -0,0 +1,17 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -march=armv8-a+fp" } */
+
+#pragma GCC push_options
+#pragma GCC target "arch=armv8-a+nofp"
+static void
+fn1 ()
+{
+}
+#pragma GCC pop_options
+float
+fn2 (float a)
+{
+ return a + 2.0;
+}
+
+/* { dg-final { scan-assembler-not "__addsf3" } } */