[Google/4-8] Support for user-guided feedback-directed library optimization

On Sun, May 11, 2014 at 11:33 AM, Xinliang David Li <davidxl@google.com> wrote:
>> ===================================================================
>> --- gcc/builtins.c    (revision 210019)
>> +++ gcc/builtins.c    (working copy)
>> @@ -87,6 +87,9 @@ static rtx result_vector (int, rtx);
>>  #endif
>>  static void expand_builtin_update_setjmp_buf (rtx);
>>  static void expand_builtin_prefetch (tree);
>> +static rtx expand_builtin_profile_invoke (tree);
>> +static rtx expand_builtin_profile_register_handler (tree);
>
> It might be more general to support handler invocation with some data
> -- basically let handler to take a 'void*' as type of the data.

As discussed offline, deferring to a follow-on patch.

>
>
>> +/* Expand a call to __builtin_profile_invoke.  Passed a pointer to
>> +   a routine that should be called on -fprofile-generate compiles
>> +   and the profile counter address and data to pass to the routine.  */
>> +
>> +static rtx
>> +expand_builtin_profile_invoke (tree exp)
>> +{
>> +  tree func, ctr, data, fndecl;
>
> Expanding late is ok for now. It might be better to support expansion
> of builin_invoke before ipa inlining (after early inline). This may
> allow more efficient code gen (i.e., when the update function is
> defined inline ..)

Ditto.

>
>> +
>> +/* Expand a call to __builtin_profile_register_handler.  Passed a pointer to
>> +   a routine that should be called atexit in -fprofile-generate binaries.  */
>> +
>> +static rtx
>> +expand_builtin_profile_register_handler (tree exp)
>> +{
>> +  tree fn, fndecl;
>> +
>
> See comment about supporting one parameter.

Ditto.

>
>> +  gcc_assert (alloc < GCOV_BLOCK_SIZE);
>> +  return alloc;
>> +}
>
>>  #if !IN_GCOV
>>  /* Write out the current block, if needs be.  */
>
>>
>> @@ -292,12 +308,11 @@ gcov_write_words (unsigned words)
>>  #if IN_LIBGCOV
>>    if (gcov_var.offset >= GCOV_BLOCK_SIZE)
>>      {
>> -      gcov_write_block (GCOV_BLOCK_SIZE);
>> -      if (gcov_var.offset)
>> -     {
>> -       gcc_assert (gcov_var.offset == 1);
>> -       memcpy (gcov_var.buffer, gcov_var.buffer + GCOV_BLOCK_SIZE, 4);
>> -     }
>> +      // Strings will cause offset to go above GCOV_BLOCK_SIZE
>> +      // by a variable amount, so we no longer assert that any
>> +      // offset be 1.
>> +      gcov_write_block (gcov_var.offset);
>> +      gcc_assert (!gcov_var.offset);
>
> This part needs more explanation.

After looking at this again, I realized I was papering over a big
issue when enabling string writing from libgcov, involving buffer
overflow, since in the libgcov case buffer is statically sized to
GCOV_BLOCK_SIZE + 1. gcov_write_block was previously only called with
"words" equal to 1 or 2, and so the offset would only exceed the block
size by at most 1. With strings we may exceed it by a variable amount,
which is obviously bad when buffer is a static size as in libgcov. I
reverted this change, and instead added handling to gcov_write_string,
to check if the buffer size would be exceeded by the new string and if
so first call gcov_write_block to open up space for the string (which
is checked to ensure it doesn't exceed the block size).

>
>> +/* Write parameter values to the gcov file. These should be applied
>> +   to profile-use compiles as macro definitions by the compiler.  */
>> +
>> +GCOV_LINKAGE void
>> +gcov_write_parameters (struct gcov_parameter_value *parameters)
>> +{
>> +  gcov_unsigned_t len = 0;
>> +  struct gcov_parameter_value *curr_parm;
>> +
>> +  if (!parameters)
>> +    return;
>> +
>> +  for (curr_parm = parameters; curr_parm;
>> +       curr_parm = curr_parm->next)
>> +    {
>
> Maybe comment here -- string followed by 2 word counter value.

Done.

>
>> +  struct gcov_parameter_value *cur_new_parm, *cur_merge_parm, *merged_parms;
>> +
>> +  merged_parms = saved_parms;
>> +
>> +  for (cur_new_parm = new_parms; cur_new_parm;
>> +       cur_new_parm = cur_new_parm->next)
>> +    {
>> +      cur_merge_parm = find_parameter (saved_parms, cur_new_parm->macro_name);
>> +
>> +      if (cur_merge_parm)
>> +        {
>> +          // Simply average them for now. The best merge strategy will
>> +          // depend on how they were computed in the first place.
>> +          cur_merge_parm->value
>> +              = (cur_merge_parm->value + cur_new_parm->value) / 2;
>
> Some parmeters may have value set that is not averageable. When
> recording parameters, maybe record the parmeter type of some sort (by
> default it is avaragble).

Deferred and added a comment.

>
>
>> +                      struct gcov_parameter_value **merged_parameters)
>>  {
>>    gcov_unsigned_t tag, length, version, stamp;
>>    unsigned t_ix, f_ix;
>> @@ -418,6 +490,15 @@ gcov_exit_merge_gcda (struct gcov_info *gi_ptr,
>>      next_summary:;
>>      }
>>
>> +  if (tag == GCOV_TAG_PARAMETERS)
>> +    {
>> +      length = gcov_read_unsigned ();
>> +      *merged_parameters = gcov_read_parameters (length);
>> +      *merged_parameters = merge_parameters (*merged_parameters,
>> +                                             gcov_parameter_values);
>> +    }
>> +  tag = gcov_read_unsigned ();
>> +
>
> Should the tag reading be inside the previous guard?

Yes, this was a bug. Ran the new patch through a profiledbootstrap as
well, which presumably would have caught this.

New patch attached. Passes regular regression tests and
profiledbootstrap+regression tests.

Thanks,
Teresa

>
> David
>
>
>
>      81,1          98%
>
>
>
> On Fri, May 9, 2014 at 1:50 PM, Teresa Johnson <tejohnson@google.com> wrote:
>> The attached patch adds support for user-guided feedback-directed
>> library optimization, for google/gcc-4_8 initially (to be ported to
>> google/gcc-4_9).
>>
>> Cc'ing Honza for any comments since I would eventually like to send
>> this and follow-on work to trunk.
>>
>> Patch to add support for user-guided feedback-directed library optimization.
>>
>> Contains support for builtins and attributes for specifying user-supplied
>> routines for initializing, updating and analyzing profile counters during
>> profile collection runs, and for feeding back macro values via a new
>> parameter section in the gcda file.
>>
>> Passes regression tests. Ok for google branches?
>>
>> Thanks,
>> Teresa
>>
>> --
>> Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413

[Google/4-8] Support for user-guided feedback-directed library optimization

Commit Message

Patch