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
