Message ID | 56D82223.8020805@foss.arm.com |
---|---|
State | New |
Headers | show |
Ping. https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00237.html Thanks, Kyrill On 03/03/16 11:38, 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? > > Thanks, > Kyrill > > P.S. The vector arguments re-layout hack that was in aarch64_set_current_function is removed because > it has become unneeded since Christian fixed some midend bugs so that it's done automatically there. > > 2016-03-03 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-03 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > PR target/70002 > PR target/69245 > * gcc.target/aarch64/pr69245_2.c: New test.
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. > 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..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); + + /* 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, + 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" } } */