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 {