Re: [google] Add options to pattern match function name for hotness attributes

[Xinliang David Li <davidxl at google dot com>]

On Sun, Jun 3, 2012 at 11:17 PM, Dehao Chen <dehao@google.com> wrote:
> I've updated the patch using deferred option.
>
> http://codereview.appspot.com/6281047
>
> Thanks,
> Dehao
>
> The new patch:
>
> 2012-06-01 ?Dehao Chen ?<dehao@google.com>
>
> ? ? ? ?* gcc/cgraph.c (cgraph_match_attribute_by_name): New function.
> ? ? ? ?(cgraph_node): Add attribute to function decl.
> ? ? ? ?* gcc/opts-global.c (add_attribute_list_to_vector): New function.
> ? ? ? ?(handle_common_deferred_options): Handle new options.
> ? ? ? ?* gcc/opts.c (common_handle_option): Handle new options.
> ? ? ? ?* gcc/opts.h (attribute_pair): New type.
> ? ? ? ?* gcc/common.opt (flag_function_attribute_list): New option.
> Index: gcc/doc/invoke.texi
> ===================================================================
> --- gcc/doc/invoke.texi (revision 188050)
> +++ gcc/doc/invoke.texi (working copy)
> @@ -362,7 +362,8 @@
> ?-fdelete-null-pointer-checks -fdse -fdevirtualize -fdse @gol
> ?-fearly-inlining -fipa-sra -fexpensive-optimizations -ffast-math @gol
> ?-ffinite-math-only -ffloat-store -fexcess-precision=@var{style} @gol
> --fforward-propagate -ffp-contract=@var{style} -ffunction-sections @gol
> +-fforward-propagate -ffp-contract=@var{style} @gol
> +-ffunction-attribute-list -ffunction-sections @gol
> ?-fgcse -fgcse-after-reload -fgcse-las -fgcse-lm -fgraphite-identity @gol
> ?-fgcse-sm -fif-conversion -fif-conversion2 -findirect-inlining @gol
> ?-finline-functions -finline-functions-called-once -finline-limit=@var{n} @gol
> @@ -8585,6 +8586,10 @@
> ?specify this option and you may have problems with debugging if
> ?you specify both this option and @option{-g}.
>
> +@item -ffunction-attribute-list
> +@opindex ffunction-attribute-list
> +List of function name patterns that will be applied specified attribute.
> +
> ?@item -fbranch-target-load-optimize
> ?@opindex fbranch-target-load-optimize
> ?Perform branch target register load optimization before prologue / epilogue
> Index: gcc/cgraph.c
> ===================================================================
> --- gcc/cgraph.c ? ? ? ?(revision 188050)
> +++ gcc/cgraph.c ? ? ? ?(working copy)
> @@ -99,6 +99,7 @@
> ?#include "ipa-utils.h"
> ?#include "lto-streamer.h"
> ?#include "l-ipo.h"
> +#include "opts.h"
>
> ?const char * const ld_plugin_symbol_resolution_names[]=
> ?{
> @@ -520,6 +521,32 @@
> ? ? }
> ?}
>
> +/* Match FNDECL's name with user specified patterns. If match is found, add
> + ? attributes to FNDECL.
> + ? name matches with pattern, iff one of the following conditions satisfy:
> + ? ? 1. strcmp (name, pattern) != 0
> + ? ? 2. pattern[len - 1] = '*' && strncmp (name, pattern, len - 1) != 0 ?*/

Fix the comment -- !strcmp ...

> +static void
> +cgraph_match_attribute_by_name (tree fndecl)
> +{
> + ?unsigned i;
> + ?attribute_pair_p p;
> + ?const char *name = lang_hooks.decl_printable_name(fndecl, 0);

It should probably use mangled name here.

> +
> + ?if (!name)
> + ? ?return;
> +
> + ?FOR_EACH_VEC_ELT (attribute_pair_p, function_attribute_list, i, p)
> + ? ?{
> + ? ? ?char *n = p->name;
> + ? ? ?int len = strlen (n);
> + ? ? ?if ((n[len - 1] == '*' && !strncmp (name, n, len - 1))
> + ? ? ? ? ?|| !strcmp (name, n))
> + ? ? ? ?decl_attributes (&fndecl, tree_cons (
> + ? ? ? ? ? ?get_identifier (p->attribute), NULL, NULL), 0);
> + ? ?}
> +}
> +
> ?/* Return cgraph node assigned to DECL. ?Create new one when needed. ?*/
>
> ?struct cgraph_node *
> @@ -554,6 +581,7 @@
> ? ? ? node->origin->nested = node;
> ? ? }
> ? cgraph_add_assembler_hash_node (node);
> + ?cgraph_match_attribute_by_name (decl);
> ? return node;
> ?}
>
> Index: gcc/opts.c
> ===================================================================
> --- gcc/opts.c ?(revision 188050)
> +++ gcc/opts.c ?(working copy)
> @@ -1647,6 +1647,10 @@
> ? ? ? /* Deferred. ?*/
> ? ? ? break;
>
> + ? ?case OPT_ffunction_attribute_list_:
> + ? ? ?/* Deferred. ?*/
> + ? ? ?break;
> +
> ? ? case OPT_fsched_verbose_:
> ?#ifdef INSN_SCHEDULING
> ? ? ? /* Handled with Var in common.opt. ?*/
> Index: gcc/opts.h
> ===================================================================
> --- gcc/opts.h ?(revision 188050)
> +++ gcc/opts.h ?(working copy)
> @@ -272,6 +272,15 @@
> ? struct cl_option_handler_func handlers[3];
> ?};
>
> +typedef struct {
> + ?char *name;
> + ?char *attribute;
> +} attribute_pair;
> +typedef attribute_pair *attribute_pair_p;
> +DEF_VEC_P(attribute_pair_p);
> +DEF_VEC_ALLOC_P(attribute_pair_p,heap);
> +extern VEC(attribute_pair_p,heap) *function_attribute_list;
> +
> ?/* Input file names. ?*/
>
> ?extern const char **in_fnames;
> Index: gcc/common.opt
> ===================================================================
> --- gcc/common.opt ? ? ?(revision 188050)
> +++ gcc/common.opt ? ? ?(working copy)
> @@ -1242,6 +1242,10 @@
> ?Common Report Var(flag_function_sections)
> ?Place each function into its own section
>
> +ffunction-attribute-list=
> +Common Joined RejectNegative Var(common_deferred_options) Defer
> +-ffunction-attribute-list=attribute:name,... ?Add attribute to named functions
> +
> ?fgcda=
> ?Common Joined RejectNegative Var(gcov_da_name)
> ?Set the gcov data file name.
> Index: gcc/opts-global.c
> ===================================================================
> --- gcc/opts-global.c ? (revision 188050)
> +++ gcc/opts-global.c ? (working copy)
> @@ -50,6 +50,8 @@
> ?const char **in_fnames;
> ?unsigned num_in_fnames;
>
> +VEC(attribute_pair_p,heap) *function_attribute_list;
> +
> ?/* Return a malloced slash-separated list of languages in MASK. ?*/
>
> ?static char *
> @@ -79,6 +81,66 @@
> ? return result;
> ?}
>
> +/* Add strings like attribute_str:name1,name2... to a char_pair_p vector. ?*/
> +
> +static void
> +add_attribute_list_to_vector (void **pvec, const char *arg)
> +{
> + ?char *tmp;
> + ?char *r;
> + ?char *w;
> + ?char *token_start;
> + ?char *attribute;
> + ?VEC(attribute_pair_p,heap) *vec = (VEC(attribute_pair_p,heap) *) *pvec;
> +
> + ?/* We never free this string. ?*/
> + ?tmp = xstrdup (arg);
> + ?attribute = tmp;
> +
> + ?for (r = tmp; *r != '\0' && *r != ':'; ++r)
> + ? ?;
> +
> + ?if (*r != ':')
> + ? ?return;

Use 'strchr'.

> +
> + ?*r = '\0';
> + ?++r;
> + ?w = r;
> + ?token_start = r;
> +
> + ?while (*r != '\0')
> + ? ?{
> + ? ? ?if (*r == ',')
> + ? ? ? {
> + ? ? ? ? attribute_pair_p p =
> + ? ? ? ? ? ? (attribute_pair_p) xmalloc (sizeof (attribute_pair));
> + ? ? ? ? p->name = token_start;
> + ? ? ? ? p->attribute = attribute;
> + ? ? ? ? *w++ = '\0';
> + ? ? ? ? ++r;
> + ? ? ? ? VEC_safe_push (attribute_pair_p, heap, vec, p);
> + ? ? ? ? token_start = w;
> + ? ? ? }
> + ? ? ?if (*r == '\\' && r[1] == ',')
> + ? ? ? {
> + ? ? ? ? *w++ = ',';
> + ? ? ? ? r += 2;
> + ? ? ? }
> + ? ? ?else
> + ? ? ? *w++ = *r++;
> + ? ?}


Simplify the code with 'strchr'.


thanks,

David

> + ?if (*token_start != '\0')
> + ? ?{
> + ? ? ?attribute_pair_p p =
> + ? ? ? ? (attribute_pair_p) xmalloc (sizeof (attribute_pair));
> + ? ? ?p->name = token_start;
> + ? ? ?p->attribute = attribute;
> + ? ? ?VEC_safe_push (attribute_pair_p, heap, vec, p);
> + ? ?}
> +
> + ?*pvec = vec;
> +}
> +
> ?/* Complain that switch DECODED does not apply to this front end (mask
> ? ?LANG_MASK). ?*/
>
> @@ -452,6 +514,10 @@
> ? ? ? ? ?set_random_seed (opt->arg);
> ? ? ? ? ?break;
>
> + ? ? ? case OPT_ffunction_attribute_list_:
> + ? ? ? ? add_attribute_list_to_vector(&function_attribute_list, opt->arg);
> + ? ? ? ? break;
> +
> ? ? ? ?case OPT_fstack_limit:
> ? ? ? ? ?/* The real switch is -fno-stack-limit. ?*/
> ? ? ? ? ?if (!opt->value)
>
> On Sun, Jun 3, 2012 at 9:14 PM, Dehao Chen <dehao@google.com> wrote:
>> Thank you guys for the comments, I'll update the patch to :
>>
>> 1. generalize the flag to enable other annotations such always_inline.
>> 2. change to use deferred option.
>>
>> Thanks,
>> Dehao
>>
>> On Sun, Jun 3, 2012 at 12:40 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Sat, Jun 2, 2012 at 11:11 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>>>> Actually Dehao also plans to teach the static predictor to understand
>>>>> standard library functions more (e.g IO functions) and add more naming
>>>>
>>>> How this differ from annotating the library?
>>>
>>> I find them more suitable to be compiler heuristic than being
>>> function's attribute -- attribute is a much stronger assertion.
>>>
>>>>
>>>> There are indeed quite some possibilities to do about library calls....
>>>>
>>>> One thing I always wondered about is how to tell predictor that paths containing
>>>> heavy IO functions don't need to be really opimized to death, since their execution
>>>> time is dominated by the IO...
>>>>
>>>
>>> Yes -- if branch predictor does the right thing and if function
>>> splitter is powerful enough, the IO code can be outlined and optimized
>>> for size :)
>>>
>>>
>>>>> based heuristics such as 'error, success, failure, fatal etc).
>>>>
>>>> yeah, this is also mentioned by some branch prediction papers. It is bit kludgy
>>>> in nature (i.e. it won't understand programs written in Czech language) but it
>>>> is an heuristics after all.
>>>>
>>>
>>> right.
>>>
>>> thanks,
>>>
>>> David
>>>
>>>> Honza
>>>>>
>>>>> thanks,
>>>>>
>>>>> David
>>>>>
>>>>> > Honza
>>>>> >>
>>>>> >> thanks,
>>>>> >>
>>>>> >> David
>>>>> >> > Honza