On Sun, Jun 3, 2012 at 11:17 PM, Dehao Chen <de...@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 <de...@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 <de...@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 <davi...@google.com>
>> wrote:
>>> On Sat, Jun 2, 2012 at 11:11 AM, Jan Hubicka <hubi...@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
