[PATCH] Add documentation for sample profiling support.

[Diego Novillo]

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.


> ================
> 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?

================
> 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/c2bef0ed/attachment.html>