Re: PR target/52555: attribute optimize is overriding command line options

[Jakub Jelinek <jakub@redhat.com>]

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