* PR target/52555: attribute optimize is overriding command line options @ 2013-02-12 0:15 Aldy Hernandez 2013-02-12 14:05 ` Jakub Jelinek 0 siblings, 1 reply; 25+ messages in thread From: Aldy Hernandez @ 2013-02-12 0:15 UTC (permalink / raw) To: Jakub Jelinek, gcc-patches [-- Attachment #1: Type: text/plain, Size: 884 bytes --] The problem here is that -ffast-math is overridden when switching optimization options on a per function basis with __attribute__((optimize("O"))). The x86 ceilf* instructions depend on unsafe math optimizations, but the optabs are created at the beginning of the compilation. When fast math becomes unavailable, the target can no longer find the instructions. Fixed by recalculating the optabs when creating new optimization nodes, and switching to these (if appropriate) at cfun switching time. How does this look? Jakub, what's this you mention in the PR about caching __optimize__((3))? You also mention I shouldn't compare against this_target_optabs, but default_target_optabs. But what if this_target_optabs has changed? (See patch). Tested on x86-64 Linux. It would be nice if someone with access to a SWITCHABLE_TARGET platform (mips) could also test this. [-- Attachment #2: curr --] [-- Type: text/plain, Size: 5413 bytes --] commit fcac0360de909e6d96fe005a3012139d487d2db8 Author: Aldy Hernandez <aldyh@redhat.com> Date: Mon Feb 11 15:51:24 2013 -0600 PR target/52555 * tree.h (struct tree_optimization_option): New field target_optabs. (TREE_OPTIMIZATION_OPTABS): New. (save_optabs_if_changed): Protoize. * optabs.h: Always declare this_target_optabs. * optabs.c (save_optabs_if_changed): New. Always declare this_target_optabs. * function.c (invoke_set_current_function_hook): Set this_target_optabs if there is one in the optimization node. c-family/ * c-common.c (handle_optimize_attribute): Call save_optabs_if_changed. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 1e6afaa..3711e69 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree args, DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = build_optimization_node (); + save_optabs_if_changed (DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)); + /* Restore current options. */ cl_optimization_restore (&global_options, &cur_opts); } diff --git a/gcc/function.c b/gcc/function.c index 4ce2259..f37e91f 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -4397,6 +4397,13 @@ invoke_set_current_function_hook (tree fndecl) { optimization_current_node = opts; cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts)); + + /* Change optabs if needed. */ + if (TREE_OPTIMIZATION_OPTABS (opts)) + this_target_optabs + = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); + else + this_target_optabs = &default_target_optabs; } targetm.set_current_function (fndecl); diff --git a/gcc/optabs.c b/gcc/optabs.c index 8a3d3a9..3ce1701 100644 --- a/gcc/optabs.c +++ b/gcc/optabs.c @@ -44,8 +44,8 @@ along with GCC; see the file COPYING3. If not see struct target_optabs default_target_optabs; struct target_libfuncs default_target_libfuncs; -#if SWITCHABLE_TARGET struct target_optabs *this_target_optabs = &default_target_optabs; +#if SWITCHABLE_TARGET struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs; #endif @@ -6184,6 +6184,41 @@ init_optabs (void) targetm.init_libfuncs (); } +/* Recompute the optabs. If they have changed, save the new set of + optabs in the optimization node OPTNODE. */ + +void +save_optabs_if_changed (tree optnode) +{ + struct target_optabs *save_target_optabs = this_target_optabs; + struct target_optabs *tmp_target_optabs = XNEW (struct target_optabs); + + /* Generate a new set of optabs into tmp_target_optabs. */ + memset (tmp_target_optabs, 0, sizeof (struct target_optabs)); + this_target_optabs = tmp_target_optabs; + init_all_optabs (); + this_target_optabs = save_target_optabs; + + /* If the optabs changed, record it in the node. */ + if (memcmp (tmp_target_optabs, this_target_optabs, + sizeof (struct target_optabs))) + { + /* ?? An existing entry in TREE_OPTIMIZATION_OPTABS indicates + multiple ((optimize)) attributes for the same function. Is + this even valid? For now, just clobber the existing entry + with the new optabs. */ + if (TREE_OPTIMIZATION_OPTABS (optnode)) + free (TREE_OPTIMIZATION_OPTABS (optnode)); + + TREE_OPTIMIZATION_OPTABS (optnode) = tmp_target_optabs; + } + else + { + TREE_OPTIMIZATION_OPTABS (optnode) = NULL; + free (tmp_target_optabs); + } +} + /* A helper function for init_sync_libfuncs. Using the basename BASE, install libfuncs into TAB for BASE_N for 1 <= N <= MAX. */ diff --git a/gcc/optabs.h b/gcc/optabs.h index c08adcf..2e8b6ec 100644 --- a/gcc/optabs.h +++ b/gcc/optabs.h @@ -76,11 +76,7 @@ struct target_optabs { }; extern struct target_optabs default_target_optabs; -#if SWITCHABLE_TARGET extern struct target_optabs *this_target_optabs; -#else -#define this_target_optabs (&default_target_optabs) -#endif \f /* Define functions given in optabs.c. */ diff --git a/gcc/testsuite/gcc.c-torture/compile/pr52555.c b/gcc/testsuite/gcc.c-torture/compile/pr52555.c new file mode 100644 index 0000000..7016834 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr52555.c @@ -0,0 +1,10 @@ +/* { dg-options "-ffast-math" } */ + +float farg; +unsigned val; + +void __attribute__((optimize("O"))) +test() +{ + val = __builtin_ceilf(farg); +} diff --git a/gcc/tree.h b/gcc/tree.h index c3c814c..eddbca8 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -3586,14 +3586,25 @@ struct GTY(()) tree_optimization_option { /* The optimization options used by the user. */ struct cl_optimization opts; + + /* Target optabs for this set of optimization options. This is of + type `struct target_optabs *'. */ + void *GTY ((skip)) target_optabs; }; #define TREE_OPTIMIZATION(NODE) \ (&OPTIMIZATION_NODE_CHECK (NODE)->optimization.opts) +#define TREE_OPTIMIZATION_OPTABS(NODE) \ + (OPTIMIZATION_NODE_CHECK (NODE)->optimization.target_optabs) + /* Return a tree node that encapsulates the current optimization options. */ extern tree build_optimization_node (void); +/* Save a new set of target_optabs in a TREE_OPTIMIZATION node if the + current set of optabs has changed. */ +extern void save_optabs_if_changed (tree); + /* Target options used by a function. */ struct GTY(()) tree_target_option { ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-12 0:15 PR target/52555: attribute optimize is overriding command line options Aldy Hernandez @ 2013-02-12 14:05 ` Jakub Jelinek 2013-02-12 15:58 ` Aldy Hernandez 2013-02-12 16:30 ` Richard Sandiford 0 siblings, 2 replies; 25+ messages in thread From: Jakub Jelinek @ 2013-02-12 14:05 UTC (permalink / raw) To: Aldy Hernandez, Richard Sandiford; +Cc: gcc-patches On Mon, Feb 11, 2013 at 06:15:05PM -0600, Aldy Hernandez wrote: > How does this look? Looks good to me. > Jakub, what's this you mention in the PR about caching > __optimize__((3))? You also mention I shouldn't compare against > this_target_optabs, but default_target_optabs. But what if > this_target_optabs has changed? (See patch). The reason for that is that this_target_optabs could at that point be simply whatever optabs used the last parsed function. this_target_optabs changes only either because of optimize attribute (not sure if MIPS as the only switchable target? supports that), or because of mips_set_mips16_mode. I think invoke_set_current_function_hook invokes the target hook after the code you've changed, so I'd say it should work fine even on MIPS. CCing Richard for that anyway. > > Tested on x86-64 Linux. It would be nice if someone with access to > a SWITCHABLE_TARGET platform (mips) could also test this. A few nits below. I'm not sure about the behavior of multiple optimize attributes either, let's try it and see how it is handled right now (ignoring optabs). > @@ -6184,6 +6184,41 @@ init_optabs (void) > targetm.init_libfuncs (); > } > > +/* Recompute the optabs. If they have changed, save the new set of > + optabs in the optimization node OPTNODE. */ > + > +void > +save_optabs_if_changed (tree optnode) > +{ > + struct target_optabs *save_target_optabs = this_target_optabs; > + struct target_optabs *tmp_target_optabs = XNEW (struct target_optabs); > + > + /* Generate a new set of optabs into tmp_target_optabs. */ > + memset (tmp_target_optabs, 0, sizeof (struct target_optabs)); I think you should just use XCNEW and drop the memset. > + this_target_optabs = tmp_target_optabs; > + init_all_optabs (); > + this_target_optabs = save_target_optabs; > + > + /* If the optabs changed, record it in the node. */ > + if (memcmp (tmp_target_optabs, this_target_optabs, > + sizeof (struct target_optabs))) > + { > + /* ?? An existing entry in TREE_OPTIMIZATION_OPTABS indicates > + multiple ((optimize)) attributes for the same function. Is > + this even valid? For now, just clobber the existing entry > + with the new optabs. */ > + if (TREE_OPTIMIZATION_OPTABS (optnode)) > + free (TREE_OPTIMIZATION_OPTABS (optnode)); > + > + TREE_OPTIMIZATION_OPTABS (optnode) = tmp_target_optabs; > + } > + else > + { > + TREE_OPTIMIZATION_OPTABS (optnode) = NULL; > + free (tmp_target_optabs); Shouldn't this (and above) be XDELETE to match the allocation style? Jakub ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-12 14:05 ` Jakub Jelinek @ 2013-02-12 15:58 ` Aldy Hernandez 2013-02-12 16:16 ` Jakub Jelinek 2013-02-12 16:30 ` Richard Sandiford 1 sibling, 1 reply; 25+ messages in thread From: Aldy Hernandez @ 2013-02-12 15:58 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Sandiford, gcc-patches [-- Attachment #1: Type: text/plain, Size: 918 bytes --] >> Jakub, what's this you mention in the PR about caching >> __optimize__((3))? You also mention I shouldn't compare against >> this_target_optabs, but default_target_optabs. But what if >> this_target_optabs has changed? (See patch). > > The reason for that is that this_target_optabs could at that point be > simply whatever optabs used the last parsed function. > this_target_optabs changes only either because of optimize attribute > (not sure if MIPS as the only switchable target? supports that), or > because of mips_set_mips16_mode. I think invoke_set_current_function_hook > invokes the target hook after the code you've changed, so I'd say it should > work fine even on MIPS. CCing Richard for that anyway. Ok, fixed. > I think you should just use XCNEW and drop the memset. Perfect. Done. > Shouldn't this (and above) be XDELETE to match the allocation style? Absolutely. Done. OK for trunk? [-- Attachment #2: curr --] [-- Type: text/plain, Size: 5359 bytes --] commit ee1f2aebe23fe0d5cecfdfde9822ef681bf6ef0c Author: Aldy Hernandez <aldyh@redhat.com> Date: Mon Feb 11 15:51:24 2013 -0600 PR target/52555 * tree.h (struct tree_optimization_option): New field target_optabs. (TREE_OPTIMIZATION_OPTABS): New. (save_optabs_if_changed): Protoize. * optabs.h: Always declare this_target_optabs. * optabs.c (save_optabs_if_changed): New. Always declare this_target_optabs. * function.c (invoke_set_current_function_hook): Set this_target_optabs if there is one in the optimization node. c-family/ * c-common.c (handle_optimize_attribute): Call save_optabs_if_changed. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 1e6afaa..3711e69 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree args, DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = build_optimization_node (); + save_optabs_if_changed (DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)); + /* Restore current options. */ cl_optimization_restore (&global_options, &cur_opts); } diff --git a/gcc/function.c b/gcc/function.c index 4ce2259..f37e91f 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -4397,6 +4397,13 @@ invoke_set_current_function_hook (tree fndecl) { optimization_current_node = opts; cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts)); + + /* Change optabs if needed. */ + if (TREE_OPTIMIZATION_OPTABS (opts)) + this_target_optabs + = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); + else + this_target_optabs = &default_target_optabs; } targetm.set_current_function (fndecl); diff --git a/gcc/optabs.c b/gcc/optabs.c index c1dacf4..11153aa 100644 --- a/gcc/optabs.c +++ b/gcc/optabs.c @@ -44,8 +44,8 @@ along with GCC; see the file COPYING3. If not see struct target_optabs default_target_optabs; struct target_libfuncs default_target_libfuncs; -#if SWITCHABLE_TARGET struct target_optabs *this_target_optabs = &default_target_optabs; +#if SWITCHABLE_TARGET struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs; #endif @@ -6207,6 +6207,40 @@ init_optabs (void) targetm.init_libfuncs (); } +/* Recompute the optabs. If they have changed, save the new set of + optabs in the optimization node OPTNODE. */ + +void +save_optabs_if_changed (tree optnode) +{ + struct target_optabs *save_target_optabs = this_target_optabs; + struct target_optabs *tmp_target_optabs = XCNEW (struct target_optabs); + + /* Generate a new set of optabs into tmp_target_optabs. */ + this_target_optabs = tmp_target_optabs; + init_all_optabs (); + this_target_optabs = save_target_optabs; + + /* If the optabs changed, record it in the node. */ + if (memcmp (tmp_target_optabs, &default_target_optabs, + sizeof (struct target_optabs))) + { + /* ?? An existing entry in TREE_OPTIMIZATION_OPTABS indicates + multiple ((optimize)) attributes for the same function. Is + this even valid? For now, just clobber the existing entry + with the new optabs. */ + if (TREE_OPTIMIZATION_OPTABS (optnode)) + XDELETE (TREE_OPTIMIZATION_OPTABS (optnode)); + + TREE_OPTIMIZATION_OPTABS (optnode) = tmp_target_optabs; + } + else + { + TREE_OPTIMIZATION_OPTABS (optnode) = NULL; + XDELETE (tmp_target_optabs); + } +} + /* A helper function for init_sync_libfuncs. Using the basename BASE, install libfuncs into TAB for BASE_N for 1 <= N <= MAX. */ diff --git a/gcc/optabs.h b/gcc/optabs.h index c08adcf..2e8b6ec 100644 --- a/gcc/optabs.h +++ b/gcc/optabs.h @@ -76,11 +76,7 @@ struct target_optabs { }; extern struct target_optabs default_target_optabs; -#if SWITCHABLE_TARGET extern struct target_optabs *this_target_optabs; -#else -#define this_target_optabs (&default_target_optabs) -#endif \f /* Define functions given in optabs.c. */ diff --git a/gcc/testsuite/gcc.c-torture/compile/pr52555.c b/gcc/testsuite/gcc.c-torture/compile/pr52555.c new file mode 100644 index 0000000..7016834 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr52555.c @@ -0,0 +1,10 @@ +/* { dg-options "-ffast-math" } */ + +float farg; +unsigned val; + +void __attribute__((optimize("O"))) +test() +{ + val = __builtin_ceilf(farg); +} diff --git a/gcc/tree.h b/gcc/tree.h index c3c814c..eddbca8 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -3586,14 +3586,25 @@ struct GTY(()) tree_optimization_option { /* The optimization options used by the user. */ struct cl_optimization opts; + + /* Target optabs for this set of optimization options. This is of + type `struct target_optabs *'. */ + void *GTY ((skip)) target_optabs; }; #define TREE_OPTIMIZATION(NODE) \ (&OPTIMIZATION_NODE_CHECK (NODE)->optimization.opts) +#define TREE_OPTIMIZATION_OPTABS(NODE) \ + (OPTIMIZATION_NODE_CHECK (NODE)->optimization.target_optabs) + /* Return a tree node that encapsulates the current optimization options. */ extern tree build_optimization_node (void); +/* Save a new set of target_optabs in a TREE_OPTIMIZATION node if the + current set of optabs has changed. */ +extern void save_optabs_if_changed (tree); + /* Target options used by a function. */ struct GTY(()) tree_target_option { ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-12 15:58 ` Aldy Hernandez @ 2013-02-12 16:16 ` Jakub Jelinek 0 siblings, 0 replies; 25+ messages in thread From: Jakub Jelinek @ 2013-02-12 16:16 UTC (permalink / raw) To: Aldy Hernandez; +Cc: Richard Sandiford, gcc-patches On Tue, Feb 12, 2013 at 09:58:38AM -0600, Aldy Hernandez wrote: > OK for trunk? I'd still prefer Richard to chime in, I'm really not familiar enough with MIPS switchable target stuff. > +/* Recompute the optabs. If they have changed, save the new set of > + optabs in the optimization node OPTNODE. */ > + > +void > +save_optabs_if_changed (tree optnode) > +{ > + struct target_optabs *save_target_optabs = this_target_optabs; > + struct target_optabs *tmp_target_optabs = XCNEW (struct target_optabs); > + > + /* Generate a new set of optabs into tmp_target_optabs. */ > + this_target_optabs = tmp_target_optabs; > + init_all_optabs (); > + this_target_optabs = save_target_optabs; > + > + /* If the optabs changed, record it in the node. */ > + if (memcmp (tmp_target_optabs, &default_target_optabs, > + sizeof (struct target_optabs))) > + { > + /* ?? An existing entry in TREE_OPTIMIZATION_OPTABS indicates > + multiple ((optimize)) attributes for the same function. Is > + this even valid? For now, just clobber the existing entry > + with the new optabs. */ > + if (TREE_OPTIMIZATION_OPTABS (optnode)) > + XDELETE (TREE_OPTIMIZATION_OPTABS (optnode)); I wonder if this necessarily won't mean that if TREE_OPTIMIZATION_OPTABS is non-NULL, then memcmp (tmp_target_optabs, TREE_OPTIMIZATION_OPTABS (optnode), sizeof (*tmp_target_optabs)) == 0. Because, optimization nodes are only shared if they contain the same set of all options. Multiple optimize attributes seems to be valid, see what handle_optimize_attribute says on them, but they together either create a fresh optimization node which certainly won't have TREE_OPTIMIZATION_OPTABS set, or they return one older, but that should be already initialized to the same thing. So perhaps start the function with if (TREE_OPTIMIZATION_OPTABS (optnode)) return; ? I.e., if you have multiple functions with the same optimization node, there is no need to do init_all_optabs again and again. Perhaps we want to have a flag whether TREE_OPTIMIZATION_OPTABS has been computed already, and don't call save_optabs_if_changed if already set, or add some magic value for TREE_OPTIMIZATION_OPTABS, where e.g. NULL would mean not computed yet, the special magic value would mean the default and other values the special optabs? Perhaps the magic value could be just &default_target_optabs... Jakub ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-12 14:05 ` Jakub Jelinek 2013-02-12 15:58 ` Aldy Hernandez @ 2013-02-12 16:30 ` Richard Sandiford 2013-02-12 17:28 ` Aldy Hernandez 1 sibling, 1 reply; 25+ messages in thread From: Richard Sandiford @ 2013-02-12 16:30 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Aldy Hernandez, gcc-patches Jakub Jelinek <jakub@redhat.com> writes: > On Mon, Feb 11, 2013 at 06:15:05PM -0600, Aldy Hernandez wrote: >> How does this look? > > Looks good to me. > >> Jakub, what's this you mention in the PR about caching >> __optimize__((3))? You also mention I shouldn't compare against >> this_target_optabs, but default_target_optabs. But what if >> this_target_optabs has changed? (See patch). > > The reason for that is that this_target_optabs could at that point be > simply whatever optabs used the last parsed function. > this_target_optabs changes only either because of optimize attribute > (not sure if MIPS as the only switchable target? supports that), or > because of mips_set_mips16_mode. I think invoke_set_current_function_hook > invokes the target hook after the code you've changed, so I'd say it should > work fine even on MIPS. CCing Richard for that anyway. The target hook won't do anything for consecutive functions that have the same mode though. It expects this_target_optabs (and other this_target_* stuff) to stay the same. Rather than: /* Change optabs if needed. */ if (TREE_OPTIMIZATION_OPTABS (opts)) this_target_optabs = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); else this_target_optabs = &default_target_optabs; I think it'd be better to have: /* Change optabs if needed. */ if (TREE_OPTIMIZATION_OPTABS (opts)) this_fn_optabs = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); else this_fn_optabs = this_target_optabs; with genopinit.c updated to use this_fn_optabs instead of this_target_optabs. Richard ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-12 16:30 ` Richard Sandiford @ 2013-02-12 17:28 ` Aldy Hernandez 2013-02-12 17:48 ` Richard Sandiford 0 siblings, 1 reply; 25+ messages in thread From: Aldy Hernandez @ 2013-02-12 17:28 UTC (permalink / raw) To: Jakub Jelinek, gcc-patches, rdsandiford [-- Attachment #1: Type: text/plain, Size: 1019 bytes --] > Rather than: > > /* Change optabs if needed. */ > if (TREE_OPTIMIZATION_OPTABS (opts)) > this_target_optabs > = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); > else > this_target_optabs = &default_target_optabs; > > I think it'd be better to have: > > /* Change optabs if needed. */ > if (TREE_OPTIMIZATION_OPTABS (opts)) > this_fn_optabs > = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); > else > this_fn_optabs = this_target_optabs; > > with genopinit.c updated to use this_fn_optabs instead of this_target_optabs. Hmmm, ok. I also added a default case setting this_fn_optabs = &default_target_optabs when the optimizations haven't changed. I can remove this if redundant. Jakub also recommended bailing if TREE_OPTIMIZATION_OPTABS already set, thus avoiding recomputing init_all_optabs() in save_optabs_if_changed: + if (TREE_OPTIMIZATION_OPTABS (optnode)) + return; Is this still part of the plan? How is this revision? [-- Attachment #2: curr --] [-- Type: text/plain, Size: 6192 bytes --] commit 48c1f4d243f81ca975b2aae34773b91ef6cbd8ca Author: Aldy Hernandez <aldyh@redhat.com> Date: Mon Feb 11 15:51:24 2013 -0600 PR target/52555 * genopinit.c (main): Use this_fn_optabs in generated init_all_optabs. * tree.h (struct tree_optimization_option): New field target_optabs. (TREE_OPTIMIZATION_OPTABS): New. (save_optabs_if_changed): Protoize. * optabs.h: Always declare this_target_optabs. Declare this_fn_optabs. * optabs.c (save_optabs_if_changed): New. Always declare this_target_optabs. Declare this_fn_optabs. * function.c (invoke_set_current_function_hook): Set this_fn_optabs if there is one in the optimization node. c-family/ * c-common.c (handle_optimize_attribute): Call save_optabs_if_changed. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 1e6afaa..3711e69 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree args, DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = build_optimization_node (); + save_optabs_if_changed (DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)); + /* Restore current options. */ cl_optimization_restore (&global_options, &cur_opts); } diff --git a/gcc/function.c b/gcc/function.c index 4ce2259..b177a98 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -4397,7 +4397,16 @@ invoke_set_current_function_hook (tree fndecl) { optimization_current_node = opts; cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts)); + + /* Change optabs if needed. */ + if (TREE_OPTIMIZATION_OPTABS (opts)) + this_fn_optabs + = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); + else + this_fn_optabs = this_target_optabs; } + else + this_fn_optabs = &default_target_optabs; targetm.set_current_function (fndecl); } diff --git a/gcc/genopinit.c b/gcc/genopinit.c index 1bb2f77..2a4b3e2 100644 --- a/gcc/genopinit.c +++ b/gcc/genopinit.c @@ -423,7 +423,7 @@ main (int argc, char **argv) fprintf (s_file, "};\n\n"); fprintf (s_file, "void\ninit_all_optabs (void)\n{\n"); - fprintf (s_file, " bool *ena = this_target_optabs->pat_enable;\n"); + fprintf (s_file, " bool *ena = this_fn_optabs->pat_enable;\n"); for (i = 0; patterns.iterate (i, &p); ++i) fprintf (s_file, " ena[%u] = HAVE_%s;\n", i, p->name); fprintf (s_file, "}\n\n"); diff --git a/gcc/optabs.c b/gcc/optabs.c index c1dacf4..e40bb5f 100644 --- a/gcc/optabs.c +++ b/gcc/optabs.c @@ -44,8 +44,9 @@ along with GCC; see the file COPYING3. If not see struct target_optabs default_target_optabs; struct target_libfuncs default_target_libfuncs; -#if SWITCHABLE_TARGET struct target_optabs *this_target_optabs = &default_target_optabs; +struct target_optabs *this_fn_optabs = &default_target_optabs; +#if SWITCHABLE_TARGET struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs; #endif @@ -6207,6 +6208,40 @@ init_optabs (void) targetm.init_libfuncs (); } +/* Recompute the optabs. If they have changed, save the new set of + optabs in the optimization node OPTNODE. */ + +void +save_optabs_if_changed (tree optnode) +{ + struct target_optabs *save_target_optabs = this_target_optabs; + struct target_optabs *tmp_target_optabs = XCNEW (struct target_optabs); + + /* Generate a new set of optabs into tmp_target_optabs. */ + this_target_optabs = tmp_target_optabs; + init_all_optabs (); + this_target_optabs = save_target_optabs; + + /* If the optabs changed, record it in the node. */ + if (memcmp (tmp_target_optabs, &default_target_optabs, + sizeof (struct target_optabs))) + { + /* ?? An existing entry in TREE_OPTIMIZATION_OPTABS indicates + multiple ((optimize)) attributes for the same function. Is + this even valid? For now, just clobber the existing entry + with the new optabs. */ + if (TREE_OPTIMIZATION_OPTABS (optnode)) + XDELETE (TREE_OPTIMIZATION_OPTABS (optnode)); + + TREE_OPTIMIZATION_OPTABS (optnode) = tmp_target_optabs; + } + else + { + TREE_OPTIMIZATION_OPTABS (optnode) = NULL; + XDELETE (tmp_target_optabs); + } +} + /* A helper function for init_sync_libfuncs. Using the basename BASE, install libfuncs into TAB for BASE_N for 1 <= N <= MAX. */ diff --git a/gcc/optabs.h b/gcc/optabs.h index c08adcf..49908d4 100644 --- a/gcc/optabs.h +++ b/gcc/optabs.h @@ -76,11 +76,8 @@ struct target_optabs { }; extern struct target_optabs default_target_optabs; -#if SWITCHABLE_TARGET extern struct target_optabs *this_target_optabs; -#else -#define this_target_optabs (&default_target_optabs) -#endif +extern struct target_optabs *this_fn_optabs; \f /* Define functions given in optabs.c. */ diff --git a/gcc/testsuite/gcc.c-torture/compile/pr52555.c b/gcc/testsuite/gcc.c-torture/compile/pr52555.c new file mode 100644 index 0000000..7016834 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr52555.c @@ -0,0 +1,10 @@ +/* { dg-options "-ffast-math" } */ + +float farg; +unsigned val; + +void __attribute__((optimize("O"))) +test() +{ + val = __builtin_ceilf(farg); +} diff --git a/gcc/tree.h b/gcc/tree.h index c3c814c..eddbca8 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -3586,14 +3586,25 @@ struct GTY(()) tree_optimization_option { /* The optimization options used by the user. */ struct cl_optimization opts; + + /* Target optabs for this set of optimization options. This is of + type `struct target_optabs *'. */ + void *GTY ((skip)) target_optabs; }; #define TREE_OPTIMIZATION(NODE) \ (&OPTIMIZATION_NODE_CHECK (NODE)->optimization.opts) +#define TREE_OPTIMIZATION_OPTABS(NODE) \ + (OPTIMIZATION_NODE_CHECK (NODE)->optimization.target_optabs) + /* Return a tree node that encapsulates the current optimization options. */ extern tree build_optimization_node (void); +/* Save a new set of target_optabs in a TREE_OPTIMIZATION node if the + current set of optabs has changed. */ +extern void save_optabs_if_changed (tree); + /* Target options used by a function. */ struct GTY(()) tree_target_option { ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-12 17:28 ` Aldy Hernandez @ 2013-02-12 17:48 ` Richard Sandiford 2013-02-12 17:46 ` Richard Sandiford 2013-02-13 17:39 ` Aldy Hernandez 0 siblings, 2 replies; 25+ messages in thread From: Richard Sandiford @ 2013-02-12 17:48 UTC (permalink / raw) To: Aldy Hernandez; +Cc: Jakub Jelinek, gcc-patches Aldy Hernandez <aldyh@redhat.com> writes: >> Rather than: >> >> /* Change optabs if needed. */ >> if (TREE_OPTIMIZATION_OPTABS (opts)) >> this_target_optabs >> = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); >> else >> this_target_optabs = &default_target_optabs; >> >> I think it'd be better to have: >> >> /* Change optabs if needed. */ >> if (TREE_OPTIMIZATION_OPTABS (opts)) >> this_fn_optabs >> = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); >> else >> this_fn_optabs = this_target_optabs; >> >> with genopinit.c updated to use this_fn_optabs instead of this_target_optabs. > > Hmmm, ok. > > I also added a default case setting this_fn_optabs = > &default_target_optabs when the optimizations haven't changed. I can > remove this if redundant. No, sounds like a good plan, but I think it should be this_target_optabs rather than &default_target_optabs. Also: @@ -76,11 +76,8 @@ struct target_optabs { }; extern struct target_optabs default_target_optabs; -#if SWITCHABLE_TARGET extern struct target_optabs *this_target_optabs; -#else -#define this_target_optabs (&default_target_optabs) -#endif This shouldn't be needed now, and: @@ -44,8 +44,9 @@ along with GCC; see the file COPYING3. If not see struct target_optabs default_target_optabs; struct target_libfuncs default_target_libfuncs; -#if SWITCHABLE_TARGET struct target_optabs *this_target_optabs = &default_target_optabs; +struct target_optabs *this_fn_optabs = &default_target_optabs; +#if SWITCHABLE_TARGET struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs; #endif I think this should be: struct target_optabs default_target_optabs; struct target_libfuncs default_target_libfuncs; +struct target_optabs *this_fn_optabs = &default_target_optabs; #if SWITCHABLE_TARGET struct target_optabs *this_target_optabs = &default_target_optabs; struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs; #endif Looks good to me otherwise as far as switchable targets go. Richard ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-12 17:48 ` Richard Sandiford @ 2013-02-12 17:46 ` Richard Sandiford 2013-02-13 17:39 ` Aldy Hernandez 1 sibling, 0 replies; 25+ messages in thread From: Richard Sandiford @ 2013-02-12 17:46 UTC (permalink / raw) To: Aldy Hernandez; +Cc: Jakub Jelinek, gcc-patches Richard Sandiford <rdsandiford@googlemail.com> writes: > Aldy Hernandez <aldyh@redhat.com> writes: >>> Rather than: >>> >>> /* Change optabs if needed. */ >>> if (TREE_OPTIMIZATION_OPTABS (opts)) >>> this_target_optabs >>> = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); >>> else >>> this_target_optabs = &default_target_optabs; >>> >>> I think it'd be better to have: >>> >>> /* Change optabs if needed. */ >>> if (TREE_OPTIMIZATION_OPTABS (opts)) >>> this_fn_optabs >>> = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); >>> else >>> this_fn_optabs = this_target_optabs; >>> >>> with genopinit.c updated to use this_fn_optabs instead of this_target_optabs. >> >> Hmmm, ok. >> >> I also added a default case setting this_fn_optabs = >> &default_target_optabs when the optimizations haven't changed. I can >> remove this if redundant. > > No, sounds like a good plan, but I think it should be this_target_optabs > rather than &default_target_optabs. Gah, just realised after sending that it would be better to have: static void invoke_set_current_function_hook (tree fndecl) { this_fn_optabs = this_target_optabs; if (!in_dummy_function) { tree opts = ((fndecl) ? DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl) : optimization_default_node); if (!opts) opts = optimization_default_node; /* Change optimization options if needed. */ if (optimization_current_node != opts) { optimization_current_node = opts; cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts)); /* Change optabs if needed. */ if (TREE_OPTIMIZATION_OPTABS (opts)) this_fn_optabs = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); } targetm.set_current_function (fndecl); } } Richard ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-12 17:48 ` Richard Sandiford 2013-02-12 17:46 ` Richard Sandiford @ 2013-02-13 17:39 ` Aldy Hernandez 2013-02-13 17:58 ` Richard Sandiford 1 sibling, 1 reply; 25+ messages in thread From: Aldy Hernandez @ 2013-02-13 17:39 UTC (permalink / raw) To: Jakub Jelinek, gcc-patches, rdsandiford [-- Attachment #1: Type: text/plain, Size: 333 bytes --] Richard. I made all the changes you suggested. I also changed other instances of s/this_target_optabs/this_fn_optabs/ which I forgot in the previous iteration. And I also changed save_optabs_if_changed() to use this_fn_optabs, since init_all_optabs() will generate the optabs into this_fn_optabs. Tested on x86-64 Linux. OK? [-- Attachment #2: curr --] [-- Type: text/plain, Size: 6694 bytes --] + PR target/52555 + * genopinit.c (main): Use this_fn_optabs in generated + init_all_optabs, raw_optab_handler, and swap_optab_enable. + * tree.h (struct tree_optimization_option): New field + target_optabs. + (TREE_OPTIMIZATION_OPTABS): New. + (save_optabs_if_changed): Protoize. + * optabs.h: Declare this_fn_optabs. + * optabs.c (save_optabs_if_changed): New. + Declare this_fn_optabs. + * function.c (invoke_set_current_function_hook): Set + this_fn_optabs if there is one in the optimization node. c-family/ + PR target/52555 + * c-common.c (handle_optimize_attribute): Call + save_optabs_if_changed. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 1e6afaa..3711e69 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree args, DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = build_optimization_node (); + save_optabs_if_changed (DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)); + /* Restore current options. */ cl_optimization_restore (&global_options, &cur_opts); } diff --git a/gcc/function.c b/gcc/function.c index 4ce2259..688242a 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -4383,6 +4383,7 @@ static bool in_dummy_function; static void invoke_set_current_function_hook (tree fndecl) { + this_fn_optabs = this_target_optabs; if (!in_dummy_function) { tree opts = ((fndecl) @@ -4397,6 +4398,11 @@ invoke_set_current_function_hook (tree fndecl) { optimization_current_node = opts; cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts)); + + /* Change optabs if needed. */ + if (TREE_OPTIMIZATION_OPTABS (opts)) + this_fn_optabs + = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); } targetm.set_current_function (fndecl); diff --git a/gcc/genopinit.c b/gcc/genopinit.c index 1bb2f77..13ebdc5 100644 --- a/gcc/genopinit.c +++ b/gcc/genopinit.c @@ -423,7 +423,7 @@ main (int argc, char **argv) fprintf (s_file, "};\n\n"); fprintf (s_file, "void\ninit_all_optabs (void)\n{\n"); - fprintf (s_file, " bool *ena = this_target_optabs->pat_enable;\n"); + fprintf (s_file, " bool *ena = this_fn_optabs->pat_enable;\n"); for (i = 0; patterns.iterate (i, &p); ++i) fprintf (s_file, " ena[%u] = HAVE_%s;\n", i, p->name); fprintf (s_file, "}\n\n"); @@ -456,7 +456,7 @@ main (int argc, char **argv) "raw_optab_handler (unsigned scode)\n" "{\n" " int i = lookup_handler (scode);\n" - " return (i >= 0 && this_target_optabs->pat_enable[i]\n" + " return (i >= 0 && this_fn_optabs->pat_enable[i]\n" " ? pats[i].icode : CODE_FOR_nothing);\n" "}\n\n"); @@ -468,8 +468,8 @@ main (int argc, char **argv) " int i = lookup_handler (scode);\n" " if (i >= 0)\n" " {\n" - " bool ret = this_target_optabs->pat_enable[i];\n" - " this_target_optabs->pat_enable[i] = set;\n" + " bool ret = this_fn_optabs->pat_enable[i];\n" + " this_fn_optabs->pat_enable[i] = set;\n" " return ret;\n" " }\n" " else\n" diff --git a/gcc/optabs.c b/gcc/optabs.c index c1dacf4..145930f 100644 --- a/gcc/optabs.c +++ b/gcc/optabs.c @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see struct target_optabs default_target_optabs; struct target_libfuncs default_target_libfuncs; +struct target_optabs *this_fn_optabs = &default_target_optabs; #if SWITCHABLE_TARGET struct target_optabs *this_target_optabs = &default_target_optabs; struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs; @@ -6207,6 +6208,40 @@ init_optabs (void) targetm.init_libfuncs (); } +/* Recompute the optabs. If they have changed, save the new set of + optabs in the optimization node OPTNODE. */ + +void +save_optabs_if_changed (tree optnode) +{ + struct target_optabs *save_fn_optabs = this_fn_optabs; + struct target_optabs *tmp_target_optabs = XCNEW (struct target_optabs); + + /* Generate a new set of optabs into tmp_target_optabs. */ + this_fn_optabs = tmp_target_optabs; + init_all_optabs (); + this_fn_optabs = save_fn_optabs; + + /* If the optabs changed, record it in the node. */ + if (memcmp (tmp_target_optabs, &default_target_optabs, + sizeof (struct target_optabs))) + { + /* ?? An existing entry in TREE_OPTIMIZATION_OPTABS indicates + multiple ((optimize)) attributes for the same function. Is + this even valid? For now, just clobber the existing entry + with the new optabs. */ + if (TREE_OPTIMIZATION_OPTABS (optnode)) + XDELETE (TREE_OPTIMIZATION_OPTABS (optnode)); + + TREE_OPTIMIZATION_OPTABS (optnode) = tmp_target_optabs; + } + else + { + TREE_OPTIMIZATION_OPTABS (optnode) = NULL; + XDELETE (tmp_target_optabs); + } +} + /* A helper function for init_sync_libfuncs. Using the basename BASE, install libfuncs into TAB for BASE_N for 1 <= N <= MAX. */ diff --git a/gcc/optabs.h b/gcc/optabs.h index c08adcf..4de4409 100644 --- a/gcc/optabs.h +++ b/gcc/optabs.h @@ -76,6 +76,7 @@ struct target_optabs { }; extern struct target_optabs default_target_optabs; +extern struct target_optabs *this_fn_optabs; #if SWITCHABLE_TARGET extern struct target_optabs *this_target_optabs; #else diff --git a/gcc/testsuite/gcc.c-torture/compile/pr52555.c b/gcc/testsuite/gcc.c-torture/compile/pr52555.c new file mode 100644 index 0000000..7016834 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr52555.c @@ -0,0 +1,10 @@ +/* { dg-options "-ffast-math" } */ + +float farg; +unsigned val; + +void __attribute__((optimize("O"))) +test() +{ + val = __builtin_ceilf(farg); +} diff --git a/gcc/tree.h b/gcc/tree.h index c3c814c..eddbca8 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -3586,14 +3586,25 @@ struct GTY(()) tree_optimization_option { /* The optimization options used by the user. */ struct cl_optimization opts; + + /* Target optabs for this set of optimization options. This is of + type `struct target_optabs *'. */ + void *GTY ((skip)) target_optabs; }; #define TREE_OPTIMIZATION(NODE) \ (&OPTIMIZATION_NODE_CHECK (NODE)->optimization.opts) +#define TREE_OPTIMIZATION_OPTABS(NODE) \ + (OPTIMIZATION_NODE_CHECK (NODE)->optimization.target_optabs) + /* Return a tree node that encapsulates the current optimization options. */ extern tree build_optimization_node (void); +/* Save a new set of target_optabs in a TREE_OPTIMIZATION node if the + current set of optabs has changed. */ +extern void save_optabs_if_changed (tree); + /* Target options used by a function. */ struct GTY(()) tree_target_option { ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-13 17:39 ` Aldy Hernandez @ 2013-02-13 17:58 ` Richard Sandiford 2013-02-13 18:08 ` Aldy Hernandez 0 siblings, 1 reply; 25+ messages in thread From: Richard Sandiford @ 2013-02-13 17:58 UTC (permalink / raw) To: Aldy Hernandez; +Cc: Jakub Jelinek, gcc-patches Aldy Hernandez <aldyh@redhat.com> writes: > Richard. > > I made all the changes you suggested. > > I also changed other instances of s/this_target_optabs/this_fn_optabs/ > which I forgot in the previous iteration. And I also changed > save_optabs_if_changed() to use this_fn_optabs, since init_all_optabs() > will generate the optabs into this_fn_optabs. Sorry, just noticed: > + /* If the optabs changed, record it in the node. */ > + if (memcmp (tmp_target_optabs, &default_target_optabs, > + sizeof (struct target_optabs))) This should be this_target_optabs rather than &default_target_optabs. Nothing but target code and initialisers should use &default_target_optabs directly. I don't think that needs a retest though. It only makes a difference on MIPS. Looks good to me otherwise. If there's any fallout on MIPS (hopefully not) I'll fix it up after the commit. Thanks, Richard ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-13 17:58 ` Richard Sandiford @ 2013-02-13 18:08 ` Aldy Hernandez 2013-02-13 19:54 ` Jakub Jelinek 0 siblings, 1 reply; 25+ messages in thread From: Aldy Hernandez @ 2013-02-13 18:08 UTC (permalink / raw) To: Jakub Jelinek, gcc-patches, rdsandiford [-- Attachment #1: Type: text/plain, Size: 590 bytes --] > Sorry, just noticed: > >> + /* If the optabs changed, record it in the node. */ >> + if (memcmp (tmp_target_optabs, &default_target_optabs, >> + sizeof (struct target_optabs))) > > This should be this_target_optabs rather than &default_target_optabs. > Nothing but target code and initialisers should use &default_target_optabs > directly. Fixed. > > I don't think that needs a retest though. It only makes a difference > on MIPS. I verified that the testcase is still fixed by this patch (just in case). Thanks so much for the review. Final patch attached. Jakub, OK? [-- Attachment #2: curr --] [-- Type: text/plain, Size: 6690 bytes --] + PR target/52555 + * genopinit.c (main): Use this_fn_optabs in generated + init_all_optabs, raw_optab_handler, and swap_optab_enable. + * tree.h (struct tree_optimization_option): New field + target_optabs. + (TREE_OPTIMIZATION_OPTABS): New. + (save_optabs_if_changed): Protoize. + * optabs.h: Declare this_fn_optabs. + * optabs.c (save_optabs_if_changed): New. + Declare this_fn_optabs. + * function.c (invoke_set_current_function_hook): Set + this_fn_optabs if there is one in the optimization node. c-family/ + PR target/52555 + * c-common.c (handle_optimize_attribute): Call + save_optabs_if_changed. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 1e6afaa..3711e69 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree args, DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = build_optimization_node (); + save_optabs_if_changed (DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node)); + /* Restore current options. */ cl_optimization_restore (&global_options, &cur_opts); } diff --git a/gcc/function.c b/gcc/function.c index 4ce2259..688242a 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -4383,6 +4383,7 @@ static bool in_dummy_function; static void invoke_set_current_function_hook (tree fndecl) { + this_fn_optabs = this_target_optabs; if (!in_dummy_function) { tree opts = ((fndecl) @@ -4397,6 +4398,11 @@ invoke_set_current_function_hook (tree fndecl) { optimization_current_node = opts; cl_optimization_restore (&global_options, TREE_OPTIMIZATION (opts)); + + /* Change optabs if needed. */ + if (TREE_OPTIMIZATION_OPTABS (opts)) + this_fn_optabs + = (struct target_optabs *) TREE_OPTIMIZATION_OPTABS (opts); } targetm.set_current_function (fndecl); diff --git a/gcc/genopinit.c b/gcc/genopinit.c index 1bb2f77..13ebdc5 100644 --- a/gcc/genopinit.c +++ b/gcc/genopinit.c @@ -423,7 +423,7 @@ main (int argc, char **argv) fprintf (s_file, "};\n\n"); fprintf (s_file, "void\ninit_all_optabs (void)\n{\n"); - fprintf (s_file, " bool *ena = this_target_optabs->pat_enable;\n"); + fprintf (s_file, " bool *ena = this_fn_optabs->pat_enable;\n"); for (i = 0; patterns.iterate (i, &p); ++i) fprintf (s_file, " ena[%u] = HAVE_%s;\n", i, p->name); fprintf (s_file, "}\n\n"); @@ -456,7 +456,7 @@ main (int argc, char **argv) "raw_optab_handler (unsigned scode)\n" "{\n" " int i = lookup_handler (scode);\n" - " return (i >= 0 && this_target_optabs->pat_enable[i]\n" + " return (i >= 0 && this_fn_optabs->pat_enable[i]\n" " ? pats[i].icode : CODE_FOR_nothing);\n" "}\n\n"); @@ -468,8 +468,8 @@ main (int argc, char **argv) " int i = lookup_handler (scode);\n" " if (i >= 0)\n" " {\n" - " bool ret = this_target_optabs->pat_enable[i];\n" - " this_target_optabs->pat_enable[i] = set;\n" + " bool ret = this_fn_optabs->pat_enable[i];\n" + " this_fn_optabs->pat_enable[i] = set;\n" " return ret;\n" " }\n" " else\n" diff --git a/gcc/optabs.c b/gcc/optabs.c index c1dacf4..00a13da 100644 --- a/gcc/optabs.c +++ b/gcc/optabs.c @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see struct target_optabs default_target_optabs; struct target_libfuncs default_target_libfuncs; +struct target_optabs *this_fn_optabs = &default_target_optabs; #if SWITCHABLE_TARGET struct target_optabs *this_target_optabs = &default_target_optabs; struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs; @@ -6207,6 +6208,40 @@ init_optabs (void) targetm.init_libfuncs (); } +/* Recompute the optabs. If they have changed, save the new set of + optabs in the optimization node OPTNODE. */ + +void +save_optabs_if_changed (tree optnode) +{ + struct target_optabs *save_fn_optabs = this_fn_optabs; + struct target_optabs *tmp_target_optabs = XCNEW (struct target_optabs); + + /* Generate a new set of optabs into tmp_target_optabs. */ + this_fn_optabs = tmp_target_optabs; + init_all_optabs (); + this_fn_optabs = save_fn_optabs; + + /* If the optabs changed, record it in the node. */ + if (memcmp (tmp_target_optabs, this_target_optabs, + sizeof (struct target_optabs))) + { + /* ?? An existing entry in TREE_OPTIMIZATION_OPTABS indicates + multiple ((optimize)) attributes for the same function. Is + this even valid? For now, just clobber the existing entry + with the new optabs. */ + if (TREE_OPTIMIZATION_OPTABS (optnode)) + XDELETE (TREE_OPTIMIZATION_OPTABS (optnode)); + + TREE_OPTIMIZATION_OPTABS (optnode) = tmp_target_optabs; + } + else + { + TREE_OPTIMIZATION_OPTABS (optnode) = NULL; + XDELETE (tmp_target_optabs); + } +} + /* A helper function for init_sync_libfuncs. Using the basename BASE, install libfuncs into TAB for BASE_N for 1 <= N <= MAX. */ diff --git a/gcc/optabs.h b/gcc/optabs.h index c08adcf..4de4409 100644 --- a/gcc/optabs.h +++ b/gcc/optabs.h @@ -76,6 +76,7 @@ struct target_optabs { }; extern struct target_optabs default_target_optabs; +extern struct target_optabs *this_fn_optabs; #if SWITCHABLE_TARGET extern struct target_optabs *this_target_optabs; #else diff --git a/gcc/testsuite/gcc.c-torture/compile/pr52555.c b/gcc/testsuite/gcc.c-torture/compile/pr52555.c new file mode 100644 index 0000000..7016834 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr52555.c @@ -0,0 +1,10 @@ +/* { dg-options "-ffast-math" } */ + +float farg; +unsigned val; + +void __attribute__((optimize("O"))) +test() +{ + val = __builtin_ceilf(farg); +} diff --git a/gcc/tree.h b/gcc/tree.h index c3c814c..eddbca8 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -3586,14 +3586,25 @@ struct GTY(()) tree_optimization_option { /* The optimization options used by the user. */ struct cl_optimization opts; + + /* Target optabs for this set of optimization options. This is of + type `struct target_optabs *'. */ + void *GTY ((skip)) target_optabs; }; #define TREE_OPTIMIZATION(NODE) \ (&OPTIMIZATION_NODE_CHECK (NODE)->optimization.opts) +#define TREE_OPTIMIZATION_OPTABS(NODE) \ + (OPTIMIZATION_NODE_CHECK (NODE)->optimization.target_optabs) + /* Return a tree node that encapsulates the current optimization options. */ extern tree build_optimization_node (void); +/* Save a new set of target_optabs in a TREE_OPTIMIZATION node if the + current set of optabs has changed. */ +extern void save_optabs_if_changed (tree); + /* Target options used by a function. */ struct GTY(()) tree_target_option { ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-13 18:08 ` Aldy Hernandez @ 2013-02-13 19:54 ` Jakub Jelinek 2013-02-15 17:23 ` Aldy Hernandez 0 siblings, 1 reply; 25+ messages in thread From: Jakub Jelinek @ 2013-02-13 19:54 UTC (permalink / raw) To: Aldy Hernandez; +Cc: gcc-patches, rdsandiford On Wed, Feb 13, 2013 at 12:07:52PM -0600, Aldy Hernandez wrote: > >Sorry, just noticed: > > > >>+ /* If the optabs changed, record it in the node. */ > >>+ if (memcmp (tmp_target_optabs, &default_target_optabs, > >>+ sizeof (struct target_optabs))) > > > >This should be this_target_optabs rather than &default_target_optabs. > >Nothing but target code and initialisers should use &default_target_optabs > >directly. > > Fixed. > > > > >I don't think that needs a retest though. It only makes a difference > >on MIPS. > > I verified that the testcase is still fixed by this patch (just in > case). Thanks so much for the review. > > Final patch attached. Actually, thinking more about SWITCHABLE_TARGETS, I don't think this works at all on those targets. this_target_optab is some random optab from whatever has been left there by previous function, it can be either the same, or different target (mips vs. mips16?). So, for SWITCHABLE_TARGETS, I'm afraid you really can't save the optabs in the optimization node (because, there could be different optabs for say all of -Ofast + mips, -Ofast + mips16, -O2 + mips and -O2 + mips16 combinations), but you probably can't save it even from within the target hook, because some non-default optimization node might be in effect. So, for SWITCHABLE_TARGETS, I think you can only save it in cfun->this_fn_optab or similar, and perhaps cache the default for mips16 only when you know the default optimization node is in effect (or force it into effect temporarily). For !SWITCHABLE_TARGETS, you can easily remember it in the optimization nodes and just copy the pointer over to cfun->this_fn_optab. And unfortunately it seems optimize attribute is supported everywhere, just the target attribute is not. But, as soon as it is reachable from cfun->this_fn_optab, we might need to GC allocate it, while optimization nodes are never freed, cfun is freed I think. Jakub ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-13 19:54 ` Jakub Jelinek @ 2013-02-15 17:23 ` Aldy Hernandez 2013-02-15 17:35 ` Jakub Jelinek 2013-02-16 11:20 ` Richard Sandiford 0 siblings, 2 replies; 25+ messages in thread From: Aldy Hernandez @ 2013-02-15 17:23 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches, rdsandiford [-- Attachment #1: Type: text/plain, Size: 828 bytes --] > Actually, thinking more about SWITCHABLE_TARGETS, I don't think this works > at all on those targets. this_target_optab is some random optab from > whatever has been left there by previous function, it can be either the Eeech... this significantly complicates things. After some further interaction off-list... attached is a patch using cfun for SWITCHABLE_TARGETs, caching the default optimization node, and using GC for the pointers. Jakub, I got rid of the suggested ifdef's because I'd prefer the compiler to compile/stress the non-common SWITCHABLE_TARGET case. All targets define SWITCHABLE_TARGET to 0/1, so there should never be an undefined symbol. If you'd prefer the ifdef, I can grudgingly change it back :). [Richard, unfortunately there are some mips specific changes.] How does this look y'all? [-- Attachment #2: curr --] [-- Type: text/plain, Size: 9980 bytes --] +2013-02-15 Aldy Hernandez <aldyh@redhat.com> + Jakub Jelinek <jakub@redhat.com> + + PR target/52555 + * genopinit.c (raw_optab_handler): Use this_fn_optabs. + (swap_optab_enable): Same. + (init_all_optabs): Use argument instead of global. + * tree.h (struct tree_optimization_option): New field + target_optabs. + * expr.h (init_all_optabs): Add argument to prototype. + (TREE_OPTIMIZATION_OPTABS): New. + (save_optabs_if_changed): Protoize. + * optabs.h: Declare this_fn_optabs. + * optabs.c (save_optabs_if_changed): New. + Declare this_fn_optabs. + (init_optabs): Add argument to init_all_optabs() call. + * function.c (invoke_set_current_function_hook): Handle per + function optabs. + * function.h (struct function): New field optabs. + * config/mips/mips.c (mips_set_mips16_mode): Handle when + optimization_current_node has changed. c/family + PR target/52555 + * c-common.c (handle_optimize_attribute): Call + save_optabs_if_changed. + diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 1e6afaa..a1d47a6 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree args, DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = build_optimization_node (); + save_optabs_if_changed (*node); + /* Restore current options. */ cl_optimization_restore (&global_options, &cur_opts); } diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index b203cdd..5e98485 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -16313,7 +16313,26 @@ mips_set_mips16_mode (int mips16_p) if (mips16_p) { if (!mips16_globals) - mips16_globals = save_target_globals (); + { + if (optimization_current_node != optimization_default_node) + { + tree opts = optimization_current_node; + /* Temporarily switch to the default optimization node, + so that *this_target_optabs is set to the default, + not reflecting whatever the first mips16 function + uses for the optimize attribute. */ + optimization_current_node = optimization_default_node; + cl_optimization_restore + (&global_options, + TREE_OPTIMIZATION (optimization_default_node)); + mips16_globals = save_target_globals (); + optimization_current_node = opts; + cl_optimization_restore (&global_options, + TREE_OPTIMIZATION (opts)); + } + else + mips16_globals = save_target_globals (); + } else restore_target_globals (mips16_globals); } diff --git a/gcc/expr.h b/gcc/expr.h index f5063b4..15fcb47 100644 --- a/gcc/expr.h +++ b/gcc/expr.h @@ -718,7 +718,7 @@ extern bool split_comparison (enum rtx_code, enum machine_mode, /* Call this once to initialize the contents of the optabs appropriately for the current target machine. */ extern void init_optabs (void); -extern void init_all_optabs (void); +extern void init_all_optabs (struct target_optabs *); /* Call this to initialize an optab function entry. */ extern rtx init_one_libfunc (const char *); diff --git a/gcc/function.c b/gcc/function.c index 4ce2259..c5eea2e 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -4400,6 +4400,31 @@ invoke_set_current_function_hook (tree fndecl) } targetm.set_current_function (fndecl); + + if (opts == optimization_default_node) + this_fn_optabs = this_target_optabs; + else + { + struct function *fn = DECL_STRUCT_FUNCTION (fndecl); + if (fn->optabs == NULL) + { + if (!SWITCHABLE_TARGET) + fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); + else + { + if (this_target_optabs == &default_target_optabs) + fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); + else + { + fn->optabs = (unsigned char *) + ggc_alloc_atomic (sizeof (struct target_optabs)); + init_all_optabs ((struct target_optabs *) fn->optabs); + } + } + } + this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs + : this_target_optabs; + } } } diff --git a/gcc/function.h b/gcc/function.h index 89d71e5..53e28b7 100644 --- a/gcc/function.h +++ b/gcc/function.h @@ -580,6 +580,9 @@ struct GTY(()) function { a string describing the reason for failure. */ const char * GTY((skip)) cannot_be_copied_reason; + /* Optabs for this function. This is of type `struct target_optabs *'. */ + unsigned char *GTY ((atomic)) optabs; + /* Collected bit flags. */ /* Number of units of general registers that need saving in stdarg diff --git a/gcc/genopinit.c b/gcc/genopinit.c index 1bb2f77..fb80717 100644 --- a/gcc/genopinit.c +++ b/gcc/genopinit.c @@ -422,8 +422,8 @@ main (int argc, char **argv) fprintf (s_file, " { %#08x, CODE_FOR_%s },\n", p->sort_num, p->name); fprintf (s_file, "};\n\n"); - fprintf (s_file, "void\ninit_all_optabs (void)\n{\n"); - fprintf (s_file, " bool *ena = this_target_optabs->pat_enable;\n"); + fprintf (s_file, "void\ninit_all_optabs (struct target_optabs *optabs)\n{\n"); + fprintf (s_file, " bool *ena = optabs->pat_enable;\n"); for (i = 0; patterns.iterate (i, &p); ++i) fprintf (s_file, " ena[%u] = HAVE_%s;\n", i, p->name); fprintf (s_file, "}\n\n"); @@ -456,7 +456,7 @@ main (int argc, char **argv) "raw_optab_handler (unsigned scode)\n" "{\n" " int i = lookup_handler (scode);\n" - " return (i >= 0 && this_target_optabs->pat_enable[i]\n" + " return (i >= 0 && this_fn_optabs->pat_enable[i]\n" " ? pats[i].icode : CODE_FOR_nothing);\n" "}\n\n"); @@ -468,8 +468,8 @@ main (int argc, char **argv) " int i = lookup_handler (scode);\n" " if (i >= 0)\n" " {\n" - " bool ret = this_target_optabs->pat_enable[i];\n" - " this_target_optabs->pat_enable[i] = set;\n" + " bool ret = this_fn_optabs->pat_enable[i];\n" + " this_fn_optabs->pat_enable[i] = set;\n" " return ret;\n" " }\n" " else\n" diff --git a/gcc/optabs.c b/gcc/optabs.c index c1dacf4..dd621c3 100644 --- a/gcc/optabs.c +++ b/gcc/optabs.c @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see struct target_optabs default_target_optabs; struct target_libfuncs default_target_libfuncs; +struct target_optabs *this_fn_optabs = &default_target_optabs; #if SWITCHABLE_TARGET struct target_optabs *this_target_optabs = &default_target_optabs; struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs; @@ -6150,7 +6151,7 @@ init_optabs (void) libfunc_hash = htab_create_ggc (10, hash_libfunc, eq_libfunc, NULL); /* Fill in the optabs with the insns we support. */ - init_all_optabs (); + init_all_optabs (this_fn_optabs); /* The ffs function operates on `int'. Fall back on it if we do not have a libgcc2 function for that width. */ @@ -6207,6 +6208,41 @@ init_optabs (void) targetm.init_libfuncs (); } +/* Recompute the optabs and save them if they have changed. */ + +void +save_optabs_if_changed (tree fndecl) +{ + /* ?? If this fails, we should temporarily restore the default + target first (set_cfun (NULL) ??), do the rest of this function, + and then restore it. */ + gcc_assert (this_target_optabs == &default_target_optabs); + + struct target_optabs *tmp_optabs = (struct target_optabs *) + ggc_alloc_atomic (sizeof (struct target_optabs)); + tree optnode = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl); + + /* Generate a new set of optabs into tmp_optabs. */ + init_all_optabs (tmp_optabs); + + /* If the optabs changed, record it. */ + if (memcmp (tmp_optabs, this_target_optabs, sizeof (struct target_optabs))) + { + /* ?? An existing optabs indicates multiple ((optimize)) + attributes for the same function. Is this even valid? For + now, just clobber the existing entry with the new optabs. */ + if (TREE_OPTIMIZATION_OPTABS (optnode)) + XDELETE (TREE_OPTIMIZATION_OPTABS (optnode)); + + TREE_OPTIMIZATION_OPTABS (optnode) = (unsigned char *) tmp_optabs; + } + else + { + TREE_OPTIMIZATION_OPTABS (optnode) = NULL; + ggc_free (tmp_optabs); + } +} + /* A helper function for init_sync_libfuncs. Using the basename BASE, install libfuncs into TAB for BASE_N for 1 <= N <= MAX. */ diff --git a/gcc/optabs.h b/gcc/optabs.h index c08adcf..4de4409 100644 --- a/gcc/optabs.h +++ b/gcc/optabs.h @@ -76,6 +76,7 @@ struct target_optabs { }; extern struct target_optabs default_target_optabs; +extern struct target_optabs *this_fn_optabs; #if SWITCHABLE_TARGET extern struct target_optabs *this_target_optabs; #else diff --git a/gcc/testsuite/gcc.c-torture/compile/pr52555.c b/gcc/testsuite/gcc.c-torture/compile/pr52555.c new file mode 100644 index 0000000..7016834 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr52555.c @@ -0,0 +1,10 @@ +/* { dg-options "-ffast-math" } */ + +float farg; +unsigned val; + +void __attribute__((optimize("O"))) +test() +{ + val = __builtin_ceilf(farg); +} diff --git a/gcc/tree.h b/gcc/tree.h index c3c814c..740d438 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -3586,14 +3586,25 @@ struct GTY(()) tree_optimization_option { /* The optimization options used by the user. */ struct cl_optimization opts; + + /* Target optabs for this set of optimization options. This is of + type `struct target_optabs *'. */ + unsigned char *GTY ((atomic)) target_optabs; }; #define TREE_OPTIMIZATION(NODE) \ (&OPTIMIZATION_NODE_CHECK (NODE)->optimization.opts) +#define TREE_OPTIMIZATION_OPTABS(NODE) \ + (OPTIMIZATION_NODE_CHECK (NODE)->optimization.target_optabs) + /* Return a tree node that encapsulates the current optimization options. */ extern tree build_optimization_node (void); +/* Save a new set of target_optabs in a TREE_OPTIMIZATION node if the + current set of optabs has changed. */ +extern void save_optabs_if_changed (tree); + /* Target options used by a function. */ struct GTY(()) tree_target_option { ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-15 17:23 ` Aldy Hernandez @ 2013-02-15 17:35 ` Jakub Jelinek 2013-02-15 17:52 ` Aldy Hernandez 2013-02-16 11:20 ` Richard Sandiford 1 sibling, 1 reply; 25+ messages in thread From: Jakub Jelinek @ 2013-02-15 17:35 UTC (permalink / raw) To: Aldy Hernandez; +Cc: gcc-patches, rdsandiford On Fri, Feb 15, 2013 at 11:23:01AM -0600, Aldy Hernandez wrote: > +2013-02-15 Aldy Hernandez <aldyh@redhat.com> > + Jakub Jelinek <jakub@redhat.com> > + > + PR target/52555 > + * genopinit.c (raw_optab_handler): Use this_fn_optabs. > + (swap_optab_enable): Same. > + (init_all_optabs): Use argument instead of global. > + * tree.h (struct tree_optimization_option): New field > + target_optabs. > + * expr.h (init_all_optabs): Add argument to prototype. > + (TREE_OPTIMIZATION_OPTABS): New. > + (save_optabs_if_changed): Protoize. > + * optabs.h: Declare this_fn_optabs. > + * optabs.c (save_optabs_if_changed): New. > + Declare this_fn_optabs. > + (init_optabs): Add argument to init_all_optabs() call. > + * function.c (invoke_set_current_function_hook): Handle per > + function optabs. > + * function.h (struct function): New field optabs. > + * config/mips/mips.c (mips_set_mips16_mode): Handle when > + optimization_current_node has changed. > c/family > + PR target/52555 > + * c-common.c (handle_optimize_attribute): Call > + save_optabs_if_changed. Looks good, just a few nits. But please wait for Richard's feedback on it. > + if (!SWITCHABLE_TARGET) > + fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); > + else > + { > + if (this_target_optabs == &default_target_optabs) > + fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); Just use if (!SWITCHABLE_TARGET || this_target_optabs == &default_target_optabs) fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); > + else > + { > + fn->optabs = (unsigned char *) > + ggc_alloc_atomic (sizeof (struct target_optabs)); > + init_all_optabs ((struct target_optabs *) fn->optabs); > + } > + } and reindent. > + } > + this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs > + : this_target_optabs; I'd prefer : here be below ? on the line above it. > + /* ?? An existing optabs indicates multiple ((optimize)) > + attributes for the same function. Is this even valid? For > + now, just clobber the existing entry with the new optabs. */ > + if (TREE_OPTIMIZATION_OPTABS (optnode)) > + XDELETE (TREE_OPTIMIZATION_OPTABS (optnode)); The comment is wrong, void foo (void) __attribute__((optimize (2))); void foo (void) __attribute__((optimize ("fast-math"))); void foo (void) __attribute__((optimize ("unroll-loops"))); void foo (void) { } is just fine, and for foo results in -O2 -ffast-math -funroll-loops options being in effect. Plus XDELETE on GC allocated memory is wrong. Just remove the comment and if and XDELETE lines. Jakub ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-15 17:35 ` Jakub Jelinek @ 2013-02-15 17:52 ` Aldy Hernandez 0 siblings, 0 replies; 25+ messages in thread From: Aldy Hernandez @ 2013-02-15 17:52 UTC (permalink / raw) To: Jakub Jelinek; +Cc: gcc-patches, rdsandiford [-- Attachment #1: Type: text/plain, Size: 1064 bytes --] > Looks good, just a few nits. But please wait for Richard's feedback on it. Will do. >> + this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs >> + : this_target_optabs; > > I'd prefer : here be below ? on the line above it. Blame emacs. I use whatever indentation it decides to use. > >> + /* ?? An existing optabs indicates multiple ((optimize)) >> + attributes for the same function. Is this even valid? For >> + now, just clobber the existing entry with the new optabs. */ >> + if (TREE_OPTIMIZATION_OPTABS (optnode)) >> + XDELETE (TREE_OPTIMIZATION_OPTABS (optnode)); > > The comment is wrong, > void foo (void) __attribute__((optimize (2))); > void foo (void) __attribute__((optimize ("fast-math"))); > void foo (void) __attribute__((optimize ("unroll-loops"))); Oh, that's just sick. Fair enough, I removed the comment altogether. > Plus XDELETE on GC allocated memory is wrong. Just remove the comment > and if and XDELETE lines. And this is just embarrassing...fixed. Thanks. Retesting the attached patch. [-- Attachment #2: curr --] [-- Type: text/plain, Size: 9725 bytes --] +2013-02-15 Aldy Hernandez <aldyh@redhat.com> + Jakub Jelinek <jakub@redhat.com> + + PR target/52555 + * genopinit.c (raw_optab_handler): Use this_fn_optabs. + (swap_optab_enable): Same. + (init_all_optabs): Use argument instead of global. + * tree.h (struct tree_optimization_option): New field + target_optabs. + * expr.h (init_all_optabs): Add argument to prototype. + (TREE_OPTIMIZATION_OPTABS): New. + (save_optabs_if_changed): Protoize. + * optabs.h: Declare this_fn_optabs. + * optabs.c (save_optabs_if_changed): New. + Declare this_fn_optabs. + (init_optabs): Add argument to init_all_optabs() call. + * function.c (invoke_set_current_function_hook): Handle per + function optabs. + * function.h (struct function): New field optabs. + * config/mips/mips.c (mips_set_mips16_mode): Handle when + optimization_current_node has changed. c/family + PR target/52555 + * c-common.c (handle_optimize_attribute): Call + save_optabs_if_changed. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 1e6afaa..a1d47a6 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree args, DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = build_optimization_node (); + save_optabs_if_changed (*node); + /* Restore current options. */ cl_optimization_restore (&global_options, &cur_opts); } diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index b203cdd..5e98485 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -16313,7 +16313,26 @@ mips_set_mips16_mode (int mips16_p) if (mips16_p) { if (!mips16_globals) - mips16_globals = save_target_globals (); + { + if (optimization_current_node != optimization_default_node) + { + tree opts = optimization_current_node; + /* Temporarily switch to the default optimization node, + so that *this_target_optabs is set to the default, + not reflecting whatever the first mips16 function + uses for the optimize attribute. */ + optimization_current_node = optimization_default_node; + cl_optimization_restore + (&global_options, + TREE_OPTIMIZATION (optimization_default_node)); + mips16_globals = save_target_globals (); + optimization_current_node = opts; + cl_optimization_restore (&global_options, + TREE_OPTIMIZATION (opts)); + } + else + mips16_globals = save_target_globals (); + } else restore_target_globals (mips16_globals); } diff --git a/gcc/expr.h b/gcc/expr.h index f5063b4..15fcb47 100644 --- a/gcc/expr.h +++ b/gcc/expr.h @@ -718,7 +718,7 @@ extern bool split_comparison (enum rtx_code, enum machine_mode, /* Call this once to initialize the contents of the optabs appropriately for the current target machine. */ extern void init_optabs (void); -extern void init_all_optabs (void); +extern void init_all_optabs (struct target_optabs *); /* Call this to initialize an optab function entry. */ extern rtx init_one_libfunc (const char *); diff --git a/gcc/function.c b/gcc/function.c index 4ce2259..c87f62d 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -4400,6 +4400,27 @@ invoke_set_current_function_hook (tree fndecl) } targetm.set_current_function (fndecl); + + if (opts == optimization_default_node) + this_fn_optabs = this_target_optabs; + else + { + struct function *fn = DECL_STRUCT_FUNCTION (fndecl); + if (fn->optabs == NULL) + { + if (!SWITCHABLE_TARGET + || this_target_optabs == &default_target_optabs) + fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); + else + { + fn->optabs = (unsigned char *) + ggc_alloc_atomic (sizeof (struct target_optabs)); + init_all_optabs ((struct target_optabs *) fn->optabs); + } + } + this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs + : this_target_optabs; + } } } diff --git a/gcc/function.h b/gcc/function.h index 89d71e5..53e28b7 100644 --- a/gcc/function.h +++ b/gcc/function.h @@ -580,6 +580,9 @@ struct GTY(()) function { a string describing the reason for failure. */ const char * GTY((skip)) cannot_be_copied_reason; + /* Optabs for this function. This is of type `struct target_optabs *'. */ + unsigned char *GTY ((atomic)) optabs; + /* Collected bit flags. */ /* Number of units of general registers that need saving in stdarg diff --git a/gcc/genopinit.c b/gcc/genopinit.c index 1bb2f77..fb80717 100644 --- a/gcc/genopinit.c +++ b/gcc/genopinit.c @@ -422,8 +422,8 @@ main (int argc, char **argv) fprintf (s_file, " { %#08x, CODE_FOR_%s },\n", p->sort_num, p->name); fprintf (s_file, "};\n\n"); - fprintf (s_file, "void\ninit_all_optabs (void)\n{\n"); - fprintf (s_file, " bool *ena = this_target_optabs->pat_enable;\n"); + fprintf (s_file, "void\ninit_all_optabs (struct target_optabs *optabs)\n{\n"); + fprintf (s_file, " bool *ena = optabs->pat_enable;\n"); for (i = 0; patterns.iterate (i, &p); ++i) fprintf (s_file, " ena[%u] = HAVE_%s;\n", i, p->name); fprintf (s_file, "}\n\n"); @@ -456,7 +456,7 @@ main (int argc, char **argv) "raw_optab_handler (unsigned scode)\n" "{\n" " int i = lookup_handler (scode);\n" - " return (i >= 0 && this_target_optabs->pat_enable[i]\n" + " return (i >= 0 && this_fn_optabs->pat_enable[i]\n" " ? pats[i].icode : CODE_FOR_nothing);\n" "}\n\n"); @@ -468,8 +468,8 @@ main (int argc, char **argv) " int i = lookup_handler (scode);\n" " if (i >= 0)\n" " {\n" - " bool ret = this_target_optabs->pat_enable[i];\n" - " this_target_optabs->pat_enable[i] = set;\n" + " bool ret = this_fn_optabs->pat_enable[i];\n" + " this_fn_optabs->pat_enable[i] = set;\n" " return ret;\n" " }\n" " else\n" diff --git a/gcc/optabs.c b/gcc/optabs.c index c1dacf4..c5778d1 100644 --- a/gcc/optabs.c +++ b/gcc/optabs.c @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see struct target_optabs default_target_optabs; struct target_libfuncs default_target_libfuncs; +struct target_optabs *this_fn_optabs = &default_target_optabs; #if SWITCHABLE_TARGET struct target_optabs *this_target_optabs = &default_target_optabs; struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs; @@ -6150,7 +6151,7 @@ init_optabs (void) libfunc_hash = htab_create_ggc (10, hash_libfunc, eq_libfunc, NULL); /* Fill in the optabs with the insns we support. */ - init_all_optabs (); + init_all_optabs (this_fn_optabs); /* The ffs function operates on `int'. Fall back on it if we do not have a libgcc2 function for that width. */ @@ -6207,6 +6208,38 @@ init_optabs (void) targetm.init_libfuncs (); } +/* Recompute the optabs and save them if they have changed. */ + +void +save_optabs_if_changed (tree fndecl) +{ + /* ?? If this fails, we should temporarily restore the default + target first (set_cfun (NULL) ??), do the rest of this function, + and then restore it. */ + gcc_assert (this_target_optabs == &default_target_optabs); + + struct target_optabs *tmp_optabs = (struct target_optabs *) + ggc_alloc_atomic (sizeof (struct target_optabs)); + tree optnode = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl); + + /* Generate a new set of optabs into tmp_optabs. */ + init_all_optabs (tmp_optabs); + + /* If the optabs changed, record it. */ + if (memcmp (tmp_optabs, this_target_optabs, sizeof (struct target_optabs))) + { + if (TREE_OPTIMIZATION_OPTABS (optnode)) + ggc_free (TREE_OPTIMIZATION_OPTABS (optnode)); + + TREE_OPTIMIZATION_OPTABS (optnode) = (unsigned char *) tmp_optabs; + } + else + { + TREE_OPTIMIZATION_OPTABS (optnode) = NULL; + ggc_free (tmp_optabs); + } +} + /* A helper function for init_sync_libfuncs. Using the basename BASE, install libfuncs into TAB for BASE_N for 1 <= N <= MAX. */ diff --git a/gcc/optabs.h b/gcc/optabs.h index c08adcf..4de4409 100644 --- a/gcc/optabs.h +++ b/gcc/optabs.h @@ -76,6 +76,7 @@ struct target_optabs { }; extern struct target_optabs default_target_optabs; +extern struct target_optabs *this_fn_optabs; #if SWITCHABLE_TARGET extern struct target_optabs *this_target_optabs; #else diff --git a/gcc/testsuite/gcc.c-torture/compile/pr52555.c b/gcc/testsuite/gcc.c-torture/compile/pr52555.c new file mode 100644 index 0000000..7016834 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr52555.c @@ -0,0 +1,10 @@ +/* { dg-options "-ffast-math" } */ + +float farg; +unsigned val; + +void __attribute__((optimize("O"))) +test() +{ + val = __builtin_ceilf(farg); +} diff --git a/gcc/tree.h b/gcc/tree.h index c3c814c..740d438 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -3586,14 +3586,25 @@ struct GTY(()) tree_optimization_option { /* The optimization options used by the user. */ struct cl_optimization opts; + + /* Target optabs for this set of optimization options. This is of + type `struct target_optabs *'. */ + unsigned char *GTY ((atomic)) target_optabs; }; #define TREE_OPTIMIZATION(NODE) \ (&OPTIMIZATION_NODE_CHECK (NODE)->optimization.opts) +#define TREE_OPTIMIZATION_OPTABS(NODE) \ + (OPTIMIZATION_NODE_CHECK (NODE)->optimization.target_optabs) + /* Return a tree node that encapsulates the current optimization options. */ extern tree build_optimization_node (void); +/* Save a new set of target_optabs in a TREE_OPTIMIZATION node if the + current set of optabs has changed. */ +extern void save_optabs_if_changed (tree); + /* Target options used by a function. */ struct GTY(()) tree_target_option { ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-15 17:23 ` Aldy Hernandez 2013-02-15 17:35 ` Jakub Jelinek @ 2013-02-16 11:20 ` Richard Sandiford 2013-02-18 18:51 ` Aldy Hernandez 1 sibling, 1 reply; 25+ messages in thread From: Richard Sandiford @ 2013-02-16 11:20 UTC (permalink / raw) To: Aldy Hernandez; +Cc: Jakub Jelinek, gcc-patches Aldy Hernandez <aldyh@redhat.com> writes: > diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c > index b203cdd..5e98485 100644 > --- a/gcc/config/mips/mips.c > +++ b/gcc/config/mips/mips.c > @@ -16313,7 +16313,26 @@ mips_set_mips16_mode (int mips16_p) > if (mips16_p) > { > if (!mips16_globals) > - mips16_globals = save_target_globals (); > + { > + if (optimization_current_node != optimization_default_node) > + { > + tree opts = optimization_current_node; > + /* Temporarily switch to the default optimization node, > + so that *this_target_optabs is set to the default, > + not reflecting whatever the first mips16 function > + uses for the optimize attribute. */ > + optimization_current_node = optimization_default_node; > + cl_optimization_restore > + (&global_options, > + TREE_OPTIMIZATION (optimization_default_node)); > + mips16_globals = save_target_globals (); > + optimization_current_node = opts; > + cl_optimization_restore (&global_options, > + TREE_OPTIMIZATION (opts)); > + } > + else > + mips16_globals = save_target_globals (); > + } I think this should go in target-globals.c, maybe as save_target_globals_default_opts. > diff --git a/gcc/function.c b/gcc/function.c > index 4ce2259..c5eea2e 100644 > --- a/gcc/function.c > +++ b/gcc/function.c > @@ -4400,6 +4400,31 @@ invoke_set_current_function_hook (tree fndecl) > } > > targetm.set_current_function (fndecl); > + > + if (opts == optimization_default_node) > + this_fn_optabs = this_target_optabs; > + else > + { > + struct function *fn = DECL_STRUCT_FUNCTION (fndecl); > + if (fn->optabs == NULL) > + { > + if (!SWITCHABLE_TARGET) > + fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); > + else > + { > + if (this_target_optabs == &default_target_optabs) > + fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); > + else > + { > + fn->optabs = (unsigned char *) > + ggc_alloc_atomic (sizeof (struct target_optabs)); > + init_all_optabs ((struct target_optabs *) fn->optabs); > + } > + } Following on from Jakub's: if (!SWITCHABLE_TARGET || this_target_optabs == &default_target_optabs) fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); I'd prefer just: if (this_target_optabs == &default_target_optabs) fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); Looks good to me otherwise, thanks. Sorry that SWITCHABLE_TARGETS has been so much hassle. TBH, like Jakub says in the PR, I was hoping things like the optimize attribute could use the target_globals stuff too. Today we just care about optabs, but e.g. arm.c has: if (TARGET_THUMB1 && optimize_size) { /* When optimizing for size on Thumb-1, it's better not to use the HI regs, because of the overhead of stacking them. */ for (regno = FIRST_HI_REGNUM; regno <= LAST_HI_REGNUM; ++regno) fixed_regs[regno] = call_used_regs[regno] = 1; } which affects other cached global state like IRA tables. The rtx_costs also often depend on optimize_size, and are cached in various places like expmed.c. E.g. for: int foo (int x, int y) { return x * 10; } the -O2 version on MIPS32 is: sll $2,$4,1 sll $4,$4,3 j $31 addu $2,$2,$4 and the -Os version is: li $2,10 j $31 mul $2,$4,$2 But even if an optimize attribute is added: int __attribute__((optimize(2))) foo (int x, int y) { return x * 10; } what you get depends on whether -Os or -O2 was passed on the command line. The attribute doesn't affect things either way. The same thing happens on x86_64. So in the end I think we'll end up trying solve the same problem that the SWITCHABLE_TARGETS stuff was trying to solve, but this time for __attribute__((optimize)). Richard ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-16 11:20 ` Richard Sandiford @ 2013-02-18 18:51 ` Aldy Hernandez 2013-02-18 23:05 ` Jakub Jelinek 0 siblings, 1 reply; 25+ messages in thread From: Aldy Hernandez @ 2013-02-18 18:51 UTC (permalink / raw) To: Jakub Jelinek, gcc-patches, rdsandiford [-- Attachment #1: Type: text/plain, Size: 1121 bytes --] On 02/16/13 05:19, Richard Sandiford wrote: > Looks good to me otherwise, thanks. Implemented all your suggestions. > > Sorry that SWITCHABLE_TARGETS has been so much hassle. TBH, like Jakub > says in the PR, I was hoping things like the optimize attribute could > use the target_globals stuff too. Today we just care about optabs, > but e.g. arm.c has: > > if (TARGET_THUMB1 && optimize_size) > { > /* When optimizing for size on Thumb-1, it's better not > to use the HI regs, because of the overhead of > stacking them. */ > for (regno = FIRST_HI_REGNUM; > regno <= LAST_HI_REGNUM; ++regno) > fixed_regs[regno] = call_used_regs[regno] = 1; > } > > which affects other cached global state like IRA tables. ... ... > So in the end I think we'll end up trying solve the same problem that > the SWITCHABLE_TARGETS stuff was trying to solve, but this time for > __attribute__((optimize)). Ughhh... perhaps this can be something for 4.9, reimplementing the optimize attribute with the target_globals infrastructure? Regstrapping attached patch. OK pending tests? [-- Attachment #2: curr --] [-- Type: text/plain, Size: 10649 bytes --] PR target/52555 * genopinit.c (raw_optab_handler): Use this_fn_optabs. (swap_optab_enable): Same. (init_all_optabs): Use argument instead of global. * tree.h (struct tree_optimization_option): New field target_optabs. * expr.h (init_all_optabs): Add argument to prototype. (TREE_OPTIMIZATION_OPTABS): New. (save_optabs_if_changed): Protoize. * optabs.h: Declare this_fn_optabs. * optabs.c (save_optabs_if_changed): New. Declare this_fn_optabs. (init_optabs): Add argument to init_all_optabs() call. * function.c (invoke_set_current_function_hook): Handle per function optabs. * function.h (struct function): New field optabs. * config/mips/mips.c (mips_set_mips16_mode): Handle when optimization_current_node has changed. * target-globals.h (save_target_globals_default_opts): Protoize. * target-globals.c (save_target_globals_default_opts): New. c/family PR target/52555 * c-common.c (handle_optimize_attribute): Call save_optabs_if_changed. diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 1e6afaa..a1d47a6 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -8925,6 +8925,8 @@ handle_optimize_attribute (tree *node, tree name, tree args, DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = build_optimization_node (); + save_optabs_if_changed (*node); + /* Restore current options. */ cl_optimization_restore (&global_options, &cur_opts); } diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index b203cdd..252e828 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -16313,7 +16313,7 @@ mips_set_mips16_mode (int mips16_p) if (mips16_p) { if (!mips16_globals) - mips16_globals = save_target_globals (); + mips16_globals = save_target_globals_default_opts (); else restore_target_globals (mips16_globals); } diff --git a/gcc/expr.h b/gcc/expr.h index f5063b4..15fcb47 100644 --- a/gcc/expr.h +++ b/gcc/expr.h @@ -718,7 +718,7 @@ extern bool split_comparison (enum rtx_code, enum machine_mode, /* Call this once to initialize the contents of the optabs appropriately for the current target machine. */ extern void init_optabs (void); -extern void init_all_optabs (void); +extern void init_all_optabs (struct target_optabs *); /* Call this to initialize an optab function entry. */ extern rtx init_one_libfunc (const char *); diff --git a/gcc/function.c b/gcc/function.c index 4ce2259..1b41cf2 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -4400,6 +4400,26 @@ invoke_set_current_function_hook (tree fndecl) } targetm.set_current_function (fndecl); + + if (opts == optimization_default_node) + this_fn_optabs = this_target_optabs; + else + { + struct function *fn = DECL_STRUCT_FUNCTION (fndecl); + if (fn->optabs == NULL) + { + if (this_target_optabs == &default_target_optabs) + fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); + else + { + fn->optabs = (unsigned char *) + ggc_alloc_atomic (sizeof (struct target_optabs)); + init_all_optabs ((struct target_optabs *) fn->optabs); + } + } + this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs + : this_target_optabs; + } } } diff --git a/gcc/function.h b/gcc/function.h index 89d71e5..53e28b7 100644 --- a/gcc/function.h +++ b/gcc/function.h @@ -580,6 +580,9 @@ struct GTY(()) function { a string describing the reason for failure. */ const char * GTY((skip)) cannot_be_copied_reason; + /* Optabs for this function. This is of type `struct target_optabs *'. */ + unsigned char *GTY ((atomic)) optabs; + /* Collected bit flags. */ /* Number of units of general registers that need saving in stdarg diff --git a/gcc/genopinit.c b/gcc/genopinit.c index 1bb2f77..fb80717 100644 --- a/gcc/genopinit.c +++ b/gcc/genopinit.c @@ -422,8 +422,8 @@ main (int argc, char **argv) fprintf (s_file, " { %#08x, CODE_FOR_%s },\n", p->sort_num, p->name); fprintf (s_file, "};\n\n"); - fprintf (s_file, "void\ninit_all_optabs (void)\n{\n"); - fprintf (s_file, " bool *ena = this_target_optabs->pat_enable;\n"); + fprintf (s_file, "void\ninit_all_optabs (struct target_optabs *optabs)\n{\n"); + fprintf (s_file, " bool *ena = optabs->pat_enable;\n"); for (i = 0; patterns.iterate (i, &p); ++i) fprintf (s_file, " ena[%u] = HAVE_%s;\n", i, p->name); fprintf (s_file, "}\n\n"); @@ -456,7 +456,7 @@ main (int argc, char **argv) "raw_optab_handler (unsigned scode)\n" "{\n" " int i = lookup_handler (scode);\n" - " return (i >= 0 && this_target_optabs->pat_enable[i]\n" + " return (i >= 0 && this_fn_optabs->pat_enable[i]\n" " ? pats[i].icode : CODE_FOR_nothing);\n" "}\n\n"); @@ -468,8 +468,8 @@ main (int argc, char **argv) " int i = lookup_handler (scode);\n" " if (i >= 0)\n" " {\n" - " bool ret = this_target_optabs->pat_enable[i];\n" - " this_target_optabs->pat_enable[i] = set;\n" + " bool ret = this_fn_optabs->pat_enable[i];\n" + " this_fn_optabs->pat_enable[i] = set;\n" " return ret;\n" " }\n" " else\n" diff --git a/gcc/optabs.c b/gcc/optabs.c index c1dacf4..c5778d1 100644 --- a/gcc/optabs.c +++ b/gcc/optabs.c @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If not see struct target_optabs default_target_optabs; struct target_libfuncs default_target_libfuncs; +struct target_optabs *this_fn_optabs = &default_target_optabs; #if SWITCHABLE_TARGET struct target_optabs *this_target_optabs = &default_target_optabs; struct target_libfuncs *this_target_libfuncs = &default_target_libfuncs; @@ -6150,7 +6151,7 @@ init_optabs (void) libfunc_hash = htab_create_ggc (10, hash_libfunc, eq_libfunc, NULL); /* Fill in the optabs with the insns we support. */ - init_all_optabs (); + init_all_optabs (this_fn_optabs); /* The ffs function operates on `int'. Fall back on it if we do not have a libgcc2 function for that width. */ @@ -6207,6 +6208,38 @@ init_optabs (void) targetm.init_libfuncs (); } +/* Recompute the optabs and save them if they have changed. */ + +void +save_optabs_if_changed (tree fndecl) +{ + /* ?? If this fails, we should temporarily restore the default + target first (set_cfun (NULL) ??), do the rest of this function, + and then restore it. */ + gcc_assert (this_target_optabs == &default_target_optabs); + + struct target_optabs *tmp_optabs = (struct target_optabs *) + ggc_alloc_atomic (sizeof (struct target_optabs)); + tree optnode = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl); + + /* Generate a new set of optabs into tmp_optabs. */ + init_all_optabs (tmp_optabs); + + /* If the optabs changed, record it. */ + if (memcmp (tmp_optabs, this_target_optabs, sizeof (struct target_optabs))) + { + if (TREE_OPTIMIZATION_OPTABS (optnode)) + ggc_free (TREE_OPTIMIZATION_OPTABS (optnode)); + + TREE_OPTIMIZATION_OPTABS (optnode) = (unsigned char *) tmp_optabs; + } + else + { + TREE_OPTIMIZATION_OPTABS (optnode) = NULL; + ggc_free (tmp_optabs); + } +} + /* A helper function for init_sync_libfuncs. Using the basename BASE, install libfuncs into TAB for BASE_N for 1 <= N <= MAX. */ diff --git a/gcc/optabs.h b/gcc/optabs.h index c08adcf..4de4409 100644 --- a/gcc/optabs.h +++ b/gcc/optabs.h @@ -76,6 +76,7 @@ struct target_optabs { }; extern struct target_optabs default_target_optabs; +extern struct target_optabs *this_fn_optabs; #if SWITCHABLE_TARGET extern struct target_optabs *this_target_optabs; #else diff --git a/gcc/target-globals.c b/gcc/target-globals.c index 533a8e5..d72495d 100644 --- a/gcc/target-globals.c +++ b/gcc/target-globals.c @@ -91,4 +91,33 @@ save_target_globals (void) return g; } +/* Like save_target_globals() above, but set *this_target_optabs + correctly when a previous function has changed + *this_target_optabs. */ + +struct target_globals * +save_target_globals_default_opts () +{ + struct target_globals *globals; + + if (optimization_current_node != optimization_default_node) + { + tree opts = optimization_current_node; + /* Temporarily switch to the default optimization node, so that + *this_target_optabs is set to the default, not reflecting + whatever a previous function used for the optimize + attribute. */ + optimization_current_node = optimization_default_node; + cl_optimization_restore + (&global_options, + TREE_OPTIMIZATION (optimization_default_node)); + globals = save_target_globals (); + optimization_current_node = opts; + cl_optimization_restore (&global_options, + TREE_OPTIMIZATION (opts)); + return globals; + } + return save_target_globals (); +} + #endif diff --git a/gcc/target-globals.h b/gcc/target-globals.h index 110f879..04eba53 100644 --- a/gcc/target-globals.h +++ b/gcc/target-globals.h @@ -60,6 +60,7 @@ struct GTY(()) target_globals { extern struct target_globals default_target_globals; extern struct target_globals *save_target_globals (void); +extern struct target_globals *save_target_globals_default_opts (void); static inline void restore_target_globals (struct target_globals *g) diff --git a/gcc/testsuite/gcc.c-torture/compile/pr52555.c b/gcc/testsuite/gcc.c-torture/compile/pr52555.c new file mode 100644 index 0000000..7016834 --- /dev/null +++ b/gcc/testsuite/gcc.c-torture/compile/pr52555.c @@ -0,0 +1,10 @@ +/* { dg-options "-ffast-math" } */ + +float farg; +unsigned val; + +void __attribute__((optimize("O"))) +test() +{ + val = __builtin_ceilf(farg); +} diff --git a/gcc/tree.h b/gcc/tree.h index c3c814c..740d438 100644 --- a/gcc/tree.h +++ b/gcc/tree.h @@ -3586,14 +3586,25 @@ struct GTY(()) tree_optimization_option { /* The optimization options used by the user. */ struct cl_optimization opts; + + /* Target optabs for this set of optimization options. This is of + type `struct target_optabs *'. */ + unsigned char *GTY ((atomic)) target_optabs; }; #define TREE_OPTIMIZATION(NODE) \ (&OPTIMIZATION_NODE_CHECK (NODE)->optimization.opts) +#define TREE_OPTIMIZATION_OPTABS(NODE) \ + (OPTIMIZATION_NODE_CHECK (NODE)->optimization.target_optabs) + /* Return a tree node that encapsulates the current optimization options. */ extern tree build_optimization_node (void); +/* Save a new set of target_optabs in a TREE_OPTIMIZATION node if the + current set of optabs has changed. */ +extern void save_optabs_if_changed (tree); + /* Target options used by a function. */ struct GTY(()) tree_target_option { ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-18 18:51 ` Aldy Hernandez @ 2013-02-18 23:05 ` Jakub Jelinek 2013-02-21 23:03 ` Steve Ellcey 0 siblings, 1 reply; 25+ messages in thread From: Jakub Jelinek @ 2013-02-18 23:05 UTC (permalink / raw) To: Aldy Hernandez; +Cc: gcc-patches, rdsandiford On Mon, Feb 18, 2013 at 12:50:59PM -0600, Aldy Hernandez wrote: > OK pending tests? > PR target/52555 > * genopinit.c (raw_optab_handler): Use this_fn_optabs. > (swap_optab_enable): Same. > (init_all_optabs): Use argument instead of global. > * tree.h (struct tree_optimization_option): New field > target_optabs. > * expr.h (init_all_optabs): Add argument to prototype. > (TREE_OPTIMIZATION_OPTABS): New. > (save_optabs_if_changed): Protoize. > * optabs.h: Declare this_fn_optabs. > * optabs.c (save_optabs_if_changed): New. > Declare this_fn_optabs. > (init_optabs): Add argument to init_all_optabs() call. > * function.c (invoke_set_current_function_hook): Handle per > function optabs. > * function.h (struct function): New field optabs. > * config/mips/mips.c (mips_set_mips16_mode): Handle when > optimization_current_node has changed. > * target-globals.h (save_target_globals_default_opts): Protoize. > * target-globals.c (save_target_globals_default_opts): New. > c/family > PR target/52555 > * c-common.c (handle_optimize_attribute): Call > save_optabs_if_changed. Yes, thanks. Jakub ^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: PR target/52555: attribute optimize is overriding command line options 2013-02-18 23:05 ` Jakub Jelinek @ 2013-02-21 23:03 ` Steve Ellcey 2013-02-22 0:10 ` Aldy Hernandez 2013-02-22 10:03 ` Jakub Jelinek 0 siblings, 2 replies; 25+ messages in thread From: Steve Ellcey @ 2013-02-21 23:03 UTC (permalink / raw) To: Jakub Jelinek, Aldy Hernandez; +Cc: gcc-patches, rdsandiford On Mon, Feb 18, 2013 at 12:50:59PM -0600, Aldy Hernandez wrote: > OK pending tests? > PR target/52555 > * genopinit.c (raw_optab_handler): Use this_fn_optabs. > (swap_optab_enable): Same. > (init_all_optabs): Use argument instead of global. > * tree.h (struct tree_optimization_option): New field > target_optabs. > * expr.h (init_all_optabs): Add argument to prototype. > (TREE_OPTIMIZATION_OPTABS): New. > (save_optabs_if_changed): Protoize. > * optabs.h: Declare this_fn_optabs. > * optabs.c (save_optabs_if_changed): New. > Declare this_fn_optabs. > (init_optabs): Add argument to init_all_optabs() call. > * function.c (invoke_set_current_function_hook): Handle per > function optabs. > * function.h (struct function): New field optabs. > * config/mips/mips.c (mips_set_mips16_mode): Handle when > optimization_current_node has changed. > * target-globals.h (save_target_globals_default_opts): Protoize. > * target-globals.c (save_target_globals_default_opts): New. > c/family > PR target/52555 > * c-common.c (handle_optimize_attribute): Call > save_optabs_if_changed. Aldy, Have you gotten any reports of problems with this patch? It seems to be sending cc1 into an infinite loop during the GCC testsuite for me. I am testing the mips-mti-linux-gnu target and tests like gcc.target/mips/call-saved-1.c are causing cc1 to suck up all my memory and swap space before the test times out. I will keep digging and see if I can figure out what is going on but I wanted to see if anyone else has reported this problem. Steve Ellcey sellcey@imgtec.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-21 23:03 ` Steve Ellcey @ 2013-02-22 0:10 ` Aldy Hernandez 2013-02-22 10:03 ` Jakub Jelinek 1 sibling, 0 replies; 25+ messages in thread From: Aldy Hernandez @ 2013-02-22 0:10 UTC (permalink / raw) To: Steve Ellcey; +Cc: Jakub Jelinek, gcc-patches, rdsandiford > Have you gotten any reports of problems with this patch? It seems to be sending cc1 into an infinite > loop during the GCC testsuite for me. I am testing the mips-mti-linux-gnu target and tests like > gcc.target/mips/call-saved-1.c are causing cc1 to suck up all my memory and swap space before the > test times out. No, I have not. > > I will keep digging and see if I can figure out what is going on but I wanted to see if anyone else has > reported this problem. > Feel free to open a PR and CC me on it. Thanks. ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-21 23:03 ` Steve Ellcey 2013-02-22 0:10 ` Aldy Hernandez @ 2013-02-22 10:03 ` Jakub Jelinek 2013-02-22 17:32 ` Steve Ellcey 2013-03-01 23:37 ` Steve Ellcey 1 sibling, 2 replies; 25+ messages in thread From: Jakub Jelinek @ 2013-02-22 10:03 UTC (permalink / raw) To: Steve Ellcey, Aldy Hernandez; +Cc: gcc-patches, rdsandiford On Thu, Feb 21, 2013 at 11:02:56PM +0000, Steve Ellcey wrote: > Have you gotten any reports of problems with this patch? It seems to be sending cc1 into an infinite > loop during the GCC testsuite for me. I am testing the mips-mti-linux-gnu target and tests like > gcc.target/mips/call-saved-1.c are causing cc1 to suck up all my memory and swap space before the > test times out. > > I will keep digging and see if I can figure out what is going on but I wanted to see if anyone else has > reported this problem. I think this should fix this (but totally untested except for call-saved-1.c, and it doesn't make any sense to test on non-mips). The problem I believe is that Aldy has changed init_optabs and insn-opinit.c to use this_fn_optabs instead of this_target_optabs, but it is only set in invoke_set_current_function_hook. During save_target_globals we want to init this_target_optabs, so we need to temporarily switch this_fn_optabs to make that happen. 2013-02-22 Jakub Jelinek <jakub@redhat.com> PR target/52555 * target-globals.c (save_target_globals): For init_reg_sets and target_reinit remporarily set this_fn_optabs to this_target_optabs. --- gcc/target-globals.c.jj 2013-02-19 07:40:03.000000000 +0100 +++ gcc/target-globals.c 2013-02-22 10:55:36.725435859 +0100 @@ -67,6 +67,7 @@ struct target_globals * save_target_globals (void) { struct target_globals *g; + struct target_optabs *saved_this_fn_optabs = this_fn_optabs; g = ggc_alloc_target_globals (); g->flag_state = XCNEW (struct target_flag_state); @@ -86,8 +87,10 @@ save_target_globals (void) g->bb_reorder = XCNEW (struct target_bb_reorder); g->lower_subreg = XCNEW (struct target_lower_subreg); restore_target_globals (g); + this_fn_optabs = this_target_optabs; init_reg_sets (); target_reinit (); + this_fn_optabs = saved_this_fn_optabs; return g; } Jakub ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-22 10:03 ` Jakub Jelinek @ 2013-02-22 17:32 ` Steve Ellcey 2013-02-22 18:17 ` Jakub Jelinek 2013-03-01 23:37 ` Steve Ellcey 1 sibling, 1 reply; 25+ messages in thread From: Steve Ellcey @ 2013-02-22 17:32 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Aldy Hernandez, gcc-patches, rdsandiford On Fri, 2013-02-22 at 11:03 +0100, Jakub Jelinek wrote: > The problem I believe is that Aldy has changed init_optabs and insn-opinit.c > to use this_fn_optabs instead of this_target_optabs, but it is only set in > invoke_set_current_function_hook. During save_target_globals we want to > init this_target_optabs, so we need to temporarily switch this_fn_optabs to > make that happen. > > 2013-02-22 Jakub Jelinek <jakub@redhat.com> > > PR target/52555 > * target-globals.c (save_target_globals): For init_reg_sets and > target_reinit remporarily set this_fn_optabs to this_target_optabs. > Jakub, I built with this patch and ran the GCC testsuite (using the target mips-mti-linux-gnu), it fixed the problem and caused no regressions for me. Steve Ellcey sellcey@imgtec.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-22 17:32 ` Steve Ellcey @ 2013-02-22 18:17 ` Jakub Jelinek 2013-02-22 19:49 ` Richard Sandiford 0 siblings, 1 reply; 25+ messages in thread From: Jakub Jelinek @ 2013-02-22 18:17 UTC (permalink / raw) To: Steve Ellcey, rdsandiford; +Cc: Aldy Hernandez, gcc-patches On Fri, Feb 22, 2013 at 09:31:54AM -0800, Steve Ellcey wrote: > On Fri, 2013-02-22 at 11:03 +0100, Jakub Jelinek wrote: > > > The problem I believe is that Aldy has changed init_optabs and insn-opinit.c > > to use this_fn_optabs instead of this_target_optabs, but it is only set in > > invoke_set_current_function_hook. During save_target_globals we want to > > init this_target_optabs, so we need to temporarily switch this_fn_optabs to > > make that happen. > > > > 2013-02-22 Jakub Jelinek <jakub@redhat.com> > > > > PR target/52555 > > * target-globals.c (save_target_globals): For init_reg_sets and > > target_reinit remporarily set this_fn_optabs to this_target_optabs. > > > > I built with this patch and ran the GCC testsuite (using the target > mips-mti-linux-gnu), it fixed the problem and caused no regressions for > me. Richard, is that ok for trunk then? Jakub ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: PR target/52555: attribute optimize is overriding command line options 2013-02-22 18:17 ` Jakub Jelinek @ 2013-02-22 19:49 ` Richard Sandiford 0 siblings, 0 replies; 25+ messages in thread From: Richard Sandiford @ 2013-02-22 19:49 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Steve Ellcey, Aldy Hernandez, gcc-patches Jakub Jelinek <jakub@redhat.com> writes: > On Fri, Feb 22, 2013 at 09:31:54AM -0800, Steve Ellcey wrote: >> On Fri, 2013-02-22 at 11:03 +0100, Jakub Jelinek wrote: >> >> > The problem I believe is that Aldy has changed init_optabs and insn-opinit.c >> > to use this_fn_optabs instead of this_target_optabs, but it is only set in >> > invoke_set_current_function_hook. During save_target_globals we want to >> > init this_target_optabs, so we need to temporarily switch this_fn_optabs to >> > make that happen. >> > >> > 2013-02-22 Jakub Jelinek <jakub@redhat.com> >> > >> > PR target/52555 >> > * target-globals.c (save_target_globals): For init_reg_sets and >> > target_reinit remporarily set this_fn_optabs to this_target_optabs. >> > >> >> I built with this patch and ran the GCC testsuite (using the target >> mips-mti-linux-gnu), it fixed the problem and caused no regressions for >> me. > > Richard, is that ok for trunk then? Looks good to me. Thanks for fixing it. Richard ^ permalink raw reply [flat|nested] 25+ messages in thread
* RE: PR target/52555: attribute optimize is overriding command line options 2013-02-22 10:03 ` Jakub Jelinek 2013-02-22 17:32 ` Steve Ellcey @ 2013-03-01 23:37 ` Steve Ellcey 1 sibling, 0 replies; 25+ messages in thread From: Steve Ellcey @ 2013-03-01 23:37 UTC (permalink / raw) To: Jakub Jelinek, Aldy Hernandez; +Cc: gcc-patches, rdsandiford Jakub and Aldy, It looks like I am having another problem with this patch. Jakubs earlier patch fixed things for me when building my mips-mti-elf target but I just started building glibc in mips16 mode with the latest compiler and I am running into this assert: mktime.c:147:1: internal compiler error: in save_optabs_if_changed, at optabs.c:6221 { ^ 0x8198e5 save_optabs_if_changed(tree_node*) /local/home/sellcey/gcc/linux_mips16/src/gcc/gcc/optabs.c:6221 0x4b090b decl_attributes(tree_node**, tree_node*, int) /local/home/sellcey/gcc/linux_mips16/src/gcc/gcc/attribs.c:545 0x4cf728 start_function(c_declspecs*, c_declarator*, tree_node*) /local/home/sellcey/gcc/linux_mips16/src/gcc/gcc/c/c-decl.c:7727 0x557114 c_common_parse_file() /local/home/sellcey/gcc/linux_mips16/src/gcc/gcc/c-family/c-opts.c:1046 I looked at this_target_optabs and it appears to be a valid pointer but it is not pointing at default_target_optabs addr and so I get the assert. I am still trying to dig out more information and create a good test case. Steve Ellcey sellcey@imgtec.com ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2013-03-01 23:37 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-02-12 0:15 PR target/52555: attribute optimize is overriding command line options Aldy Hernandez 2013-02-12 14:05 ` Jakub Jelinek 2013-02-12 15:58 ` Aldy Hernandez 2013-02-12 16:16 ` Jakub Jelinek 2013-02-12 16:30 ` Richard Sandiford 2013-02-12 17:28 ` Aldy Hernandez 2013-02-12 17:48 ` Richard Sandiford 2013-02-12 17:46 ` Richard Sandiford 2013-02-13 17:39 ` Aldy Hernandez 2013-02-13 17:58 ` Richard Sandiford 2013-02-13 18:08 ` Aldy Hernandez 2013-02-13 19:54 ` Jakub Jelinek 2013-02-15 17:23 ` Aldy Hernandez 2013-02-15 17:35 ` Jakub Jelinek 2013-02-15 17:52 ` Aldy Hernandez 2013-02-16 11:20 ` Richard Sandiford 2013-02-18 18:51 ` Aldy Hernandez 2013-02-18 23:05 ` Jakub Jelinek 2013-02-21 23:03 ` Steve Ellcey 2013-02-22 0:10 ` Aldy Hernandez 2013-02-22 10:03 ` Jakub Jelinek 2013-02-22 17:32 ` Steve Ellcey 2013-02-22 18:17 ` Jakub Jelinek 2013-02-22 19:49 ` Richard Sandiford 2013-03-01 23:37 ` Steve Ellcey
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).