Final Ping: Function specific patch against the mainline

["Steven Bosscher" <stevenb dot gcc at gmail dot com>]

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):

> +@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! ;-)

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


> +  /* 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.




> +/* 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.


> +  /* 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);
+}


> 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


> +/* 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 ;-)


> +  /* 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.


> +      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
;-)


> +/* 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.


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 :-)

Gr.
Steven