| Message ID | 51198989.7090803@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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 /* 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 {
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. 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.