Re: Final Ping: Function specific patch against the mainline

[Michael Meissner <gnu at the-meissners dot org>]

On Sun, Jul 13, 2008 at 12:09:07PM +0200, Steven Bosscher wrote:
> xf. http://gcc.gnu.org/ml/gcc-patches/2008-07/msg00904.html
> 
> > In the absence of any further comments about my function specific patches, I
> > plan to check these into the mainline in a few days.
> 
> 
> The code and the documentation don't match,
> DECL_FUNCTION_SPECIFIC_OPTIMIZATION seems to return a tree, not a
> struct cl_optimization (sadly):

Thanks for pointing that out.  I will reconcile it.

> > +@item DECL_FUNCTION_SPECIFIC_OPTIMIZATION
> > +This macro returns the pointer to a structure of type
> > +@code{struct cl_optimization} that holds the optimization options used
> > +by the function.
> >  @end ftable
> 
> > +  tree caller_tree = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (caller);
> > +  tree callee_tree = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (callee);
> 
> > +  /* Function specific options that are used by this function.  */
> > +  tree function_specific_target;	/* target options */
> > +  tree function_specific_optimization;	/* optimization options */
> 
> DECL_FUNCTION_SPECIFIC_TARGET may have the same inconsistency, I
> haven't checked.  You seem to have wrapped the cl_optimization structs
> in a tree, with a field that is retrieved with the TREE_OPTIMIZATION
> macro:
> > +  struct cl_optimization *prev = TREE_OPTIMIZATION (prev_tree);
> > +  struct cl_optimization *cur  = TREE_OPTIMIZATION (cur_tree);
> 
> Why are all these new nodes 'tree' nodes?  Is there any reason to not
> make them something else?  I think we should avoid any step back in
> the direction of the old "everything is a tree" world, if we can! ;-)

Originally I had them as separate structures, but at the time I had problems
getting the garbage collection support to work in all cases (hence my
forgetting to change the documentation when I put them into trees).  I can look
at changing these back to just the structs again.  I do think we need to record
this information so that it will survive with LTO, whether it be tree or plain
structure.

> How is this patch going to interact with tuples?  I couldn't figure
> out whether you put fake statements into the IL.

In this stage of the function specific options, there are no statements added
to the IL.  It is anticipated that in the future where we could add the ability
to have clone functions that are compiled with different options and then
dispatched depending on the options of the machine you are running on, but I
would hope tuples would already have been merged at that point.

> 
> > +  /* If this is a function, do we have a pragma optimization to set the
> > +     optimization flags add it to the optimization attribute.  */
> 
> This is not English.

Thanks.

> > +/* Determine whether a function FN can be inlined.  Be conservative, and assume
> > +   any function with different target specific options cannot be inlined.  */
> 
> That does seem very conservative, yes.  Worth documenting in the
> internals manual IMHO.

I can put in a sentence about the default behavior if the hook isn't defined.

> 
> > +  /* If callee has no option attributes, then it is ok to inline */
> > +  if (!callee_opts)
> > +    ret = true;
> 
>   /* If callee has no option attributes, then it is ok to inline.  */
>   if (!callee_opts)
>     return true;
> 
> But actually this whole function can be collapsed to:
> +/* Determine whether a function FN can be inlined.  Be conservative, and assume
> +   any function with different target specific options cannot be inlined.  */
> +
> +bool
> +default_can_inline_p (tree caller, tree callee)
> +{
> +  tree callee_opts = DECL_FUNCTION_SPECIFIC_TARGET (callee);
> +  tree caller_opts = DECL_FUNCTION_SPECIFIC_TARGET (caller);
> +
> +  /* If callee and caller have no options, then it is ok to inline.  Both
> +     CALLEE_OPTS and CALLER_OPTS will be NULL.
> +
> +     If both caller and callee have attributes: Because we don't know anything
> +     about target specific options, assume if we get here the port hashes the
> +     target specific information to a common pointer, and if the pointers are
> +     the same, then they are the same target.  */
> +   return (callee_opts == caller_opts);
> +}

No, that is incorrect.  If the caller is defined with function specific option
but the callee is a generic function without function specific options, then
you want the inlining to proceed.

> 
> > Index: gcc/flags.h
> > ...+/* Unset all of the options that are normally off for -Os.  This is to allow
> > +   targets to have a function specific option that optimizes for space for a
> > +   particular function.  It is assumed that the current options have been saved
> > +   by a call to cl_optimization_save and will be restored by a call to
> > +   cl_optimization_restore at the end of the function.  */
> > +extern void optimize_for_space (void);
> >  #endif /* ! GCC_FLAGS_H */
> 
> I think we usually don't put interface comments in the header.  Also,
> I think this belongs in opts.h, not flags.h

Or possibly eliminated, since the only caller is decode_options.  Originally
this was added as a hook for the cold functions to change to space
optimization, but when I fleshed out the ability to set optimization options as
well as target options on a per-function basis, I switched hot/cold attributes
to use that.  The reason why flags.h was used, was the other caller (now
deleted) did not include opts.h.

> 
> > +/* Hash table and temporary node for optimization flags and target option
> > +   flags.  Use the same hash table for both sets of options.  */
> > +static GTY (()) tree cl_optimization_node;
> > +static GTY (()) tree cl_target_option_node;
> > +static GTY ((if_marked ("ggc_marked_p"), param_is (union tree_node)))
> > +     htab_t cl_option_hash_table;
> 
> What are the temporary nodes for?  Deserves a comment.  Temporary
> variables are not usually globals, after all ;-)

These are used the same way as the constant integer nodes are in the same
source file.  The compiler keeps one node lying around for building the
options, and if the options hash to a previously allocated set of options, it
uses that.  Otherwise it uses the allocated node, and then allocates a new
temporary node.  The assumption is almost all calls will result in the same
options, and you avoid having to allocate/free nodes.  It does warrant an extra
comment though.

> 
> > +  /* Function to validate the attribute((option(...))) strings or NULL.  If the
> > +     option is validated, it is assumed that DECL_FUNCTION_SPECIFIC will be
> > +     filled in in the function decl node.  */
> > +  bool (*valid_option_attribute_p) (tree, tree, tree, int);
> > +
> > +  /* Function to save any extra target state in the target options
> > +     structure.  */
> > +  void (*target_option_save) (struct cl_target_option *);
> > +
> > +  /* Function to restore any extra target state from the target options
> > +     structure.  */
> > +  void (*target_option_restore) (struct cl_target_option *);
> > +
> > +  /* Function to print any extra target state from the target options
> > +     structure.  */
> > +  void (*target_option_print) (FILE *, int, struct cl_target_option *);
> > +
> > +  /* Function to parse arguments to be validated for #pragma option, and to
> > +     change the state if the options are valid.  If the arguments are NULL, use
> > +     the default target options.  Return true if the options are valid, and set
> > +     the current state.  */
> > +  bool (*target_option_pragma_parse) (tree);
> 
> How about wrapping these in a sub-structure 'target_option', like
> emutls, vectorize, calls, etc.

I had done it in the past.  It isn't that big of a deal to add a sub-structure.

> 
> > +      if (!tree_can_inline_p (edge->caller->decl, edge->callee->decl))
> > +	{
> > +	  CALL_STMT_CANNOT_INLINE_P (edge->call_stmt) = true;
> > +	  edge->inline_failed = N_("target specific option mismatch");
> > +	  if (dump_file)
> > +	    fprintf (dump_file, " inline_failed:%s.\n", edge->inline_failed);
> > +	  continue;
> > +	}
> 
> > +/* Return whether it is safe to inline a function because it used different
> > +   target specific options or different optimization options.  */
> > +bool
> > +tree_can_inline_p (tree caller, tree callee)
> 
> This new 'tree_can_inline_p' only looks at target specific
> optimizations.  I think the function name is confusing, you should
> rename it IMHO.  Maybe something like
> 'function_optimization_opts_compatible_for_inlining' or something
> more descriptive of what the function actually checks.
> 'tree_can_inline_p' is definitely not descriptive enough for my taste
> ;-)

However, since it calls the backend hook, the backend can do other checks that
are not related to function specific options.  Note, the machine independent
tree_can_inline_p does check the optimization level, so it isn't just target
specific options.

> 
> > +/* Function specific option attribute support.  */
> > +#ifndef TARGET_VALID_OPTION_ATTRIBUTE_P
> > +#define TARGET_VALID_OPTION_ATTRIBUTE_P NULL
> > +#endif
> 
> The preferred style these days is to always provide a default hook, to
> prevent the "if (targetm.blah) targetm.blah(...)" idiom as much as
> possible. With a default hook, you can just call it.  It's OK if the
> default hook is a nop.  See the hook_* functions in targhooks.c.

We need to check whether the backend defined the hook in the first place to
give the error message that attribute((option(...))) is not supported for this
port.  Having it be NULL made the check easier than testing if the hook was
equal to the default hook.

> 
> Looks very nice to me, overall.  The only thing I really dislike, is
> how the options are stored as a 'tree'.  I'm guessing you have to,
> because the attributes handling is still not converted to something
> that doesn't use 'tree'.  But oh well, that's something we could
> address later, as usual :-)

The attribute handling has nothing to do with these fields.  No matter how the
attributes are stored, you want a binary blob to hold the options you need for
the function so that you don't have to continually re-parse the attributes.

-- 
Michael Meissner
email: gnu@the-meissners.org
http://www.the-meissners.org