[PATCH] Add documentation for sample profiling support.

[Richard Smith]

On Tue, Apr 22, 2014 at 2:55 PM, Diego Novillo <dnovillo at google.com> wrote:

>
>
>
> On Mon, Apr 21, 2014 at 5:41 PM, Richard Smith <richard at metafoo.co.uk>wrote:
>
>>
>>   Mostly copy-editing, but some questions too.
>>
>>
>> ================
>> Comment at: docs/UsersManual.rst:1073
>> @@ +1072,3 @@
>> +hardware counters, while your application executes. They are typically
>> +very efficient and do not incur in a large runtime overhead. The
>> +sample data collected by the profiler can be used during compilation
>> ----------------
>> Should this be "do not incur a large runtime overhead" ?
>>
>
> Done.
>
>
>>
>> ================
>> Comment at: docs/UsersManual.rst:1075
>> @@ +1074,3 @@
>> +sample data collected by the profiler can be used during compilation
>> +to determine what are the most executed areas of the code.
>> +
>> ----------------
>> "to determine what the most executed ares of the code are."
>>
>>
> Done.
>
>
>> ================
>> Comment at: docs/UsersManual.rst:1078
>> @@ +1077,3 @@
>> +In particular, sample profilers can provide execution counts for all
>> +instructions in the code, information on branches taken and function
>> +invocation. The compiler can use this information in its optimization
>> ----------------
>> "and" instead of ",".
>>
>>
> Done.
>
>
>> ================
>> Comment at: docs/UsersManual.rst:1083
>> @@ +1082,3 @@
>> +basic blocks. Knowing that a function ``foo`` is called more
>> +frequently than another ``bar`` helps the inliner.
>> +
>> ----------------
>> "another function ``bar``"
>>
>>
> Done.
>
> ================
>> Comment at: docs/UsersManual.rst:1092
>> @@ +1091,3 @@
>> +   usual build flags that you always build your application with. The
>> only
>> +   requirement is that you add ``-gline-tables-ony`` or ``-g`` to the
>> +   command line. This is important for the profiler to be able to map
>> ----------------
>> "``-gline-tables-only``"
>>
>>
> Done.
>
>
>> ================
>> Comment at: docs/UsersManual.rst:1131-1133
>> @@ +1130,5 @@
>> +
>> +4. Build the code again using the collected profile. This step feeds
>> +   the profile back to the optimizers. This should result in a binary
>> +   that executes faster than the original one.
>> +
>> ----------------
>> Are you required to use the exact same arguments that you used before,
>> other than the `-fprofile-sample-use=` argument? Either way, we should say
>> so here.
>>
>>
> Done.
>
>
>
>> ================
>> Comment at: docs/UsersManual.rst:1165
>> @@ +1164,3 @@
>> +at the prologue of the function (second number). This head sample
>> +count provides an indicator of how frequent is the function invoked.
>> +
>> ----------------
>> "how frequently the function is invoked."
>>
>>
> Done.
>
>
>> ================
>> Comment at: docs/UsersManual.rst:1164
>> @@ +1163,3 @@
>> +function (first number), and the total number of samples accumulated
>> +at the prologue of the function (second number). This head sample
>> +count provides an indicator of how frequent is the function invoked.
>> ----------------
>> Should "at the prologue" be "in the prologue"?
>>
>>
> Done.
>
>
>> ================
>> Comment at: docs/UsersManual.rst:1176-1178
>> @@ +1175,5 @@
>> +
>> +b. [OPTIONAL] Discriminator. This is used if the sampled program
>> +   was compiled with DWARF discriminator support
>> +   (http://wiki.dwarfstd.org/index.php?title=Path_Discriminators)
>> +
>> ----------------
>> ... and what is it, in that case? At a guess: "If present, this is a
>> decimal integer representing the value of the DWARF discriminator register."
>>
>>
> Done.
>

Thanks, this helps -- it explains why we'd want this. But it still doesn't
answer my question of what value is specified here, and how it's
represented.

================
>> Comment at: docs/UsersManual.rst:1173-1174
>> @@ +1172,4 @@
>> +   always relative to the line where symbol of the function is
>> +   defined. So, if the function has its header at line 280, the offset
>> +   13 is at line 293 in the file.
>> +
>> ----------------
>> What happens if the line is in a different file (eg through macro
>> expansion or inlining)?
>>
>>
> It will break down then. Though I don't think I have much sympathy for
> something like:
>
> file1.h:
>    int foo() {
>      i1;
>      i2;
> ------------
> file2.h:
>     i3;
>     i4;
> }
>
> Or are you thinking something else?
>

More mundane cases than that =) For instance:

file.def:
  FOO(a)
  FOO(b)
  FOO(c)
  #undef FOO

file.cc:
  void f() {
    #define FOO(x) x = 0;
    #include "file.def"
  }

or simply:

file.cc:
  void f() {
    assert(some_expensive_call());
  }

================
>> Comment at: docs/UsersManual.rst:1180-1181
>> @@ +1179,4 @@
>> +
>> +c. Number of samples. This is the number of samples collected by
>> +   the profiler at this source location.
>> +
>> ----------------
>> Decimal integer? Floating point? Something else? Is scientific notation
>> OK? Is hexadecimal OK?
>>
>>
> Done.
>
>
>> ================
>> Comment at: docs/UsersManual.rst:1149-1150
>> @@ +1148,4 @@
>> +which correspond to each of the functions executed at runtime. Each
>> +section has the following format (taken from
>> +https://github.com/google/autofdo/blob/master/profile_writer.h):
>> +
>> ----------------
>> Is the whitespace required to be a single space, or can it be any
>> sequence of horizontal whitespace? Is any other whitespace allowed than
>> that listed here?
>>
>>
> Done.
>
>
>
>> ================
>> Comment at: docs/UsersManual.rst:1193-1194
>> @@ +1192,4 @@
>> +   The above means that at relative line offset 130 there is a call
>> +   instruction that calls one of ``foo()``, ``bar()`` and ``baz()``.
>> +   With ``baz()`` being the relatively more frequent call target.
>> +
>> ----------------
>> ". With" -> ", with"
>> "frequent call" -> "frequently called"
>>
>>
> Done.
>
>
> Thanks for the feedback!
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140422/ac2a0872/attachment.html>