[PATCH] D13107: Support for function summary index bitcode sections and files

[Teresa Johnson via llvm-commits]

On Mon, Oct 5, 2015 at 5:08 PM, Eric Christopher <echristo at gmail.com> wrote:
> echristo added a subscriber: echristo.
> echristo added a comment.
>
> Hi Teresa,
>
> A couple of inline comments, I'll have more, but I just thought I'd send a couple out.

Thanks for the comments! Responses below.

>
> One general comment:
>
> How expensive is the index (size and time to compute)? Is it worth always including in the module?

Should be negligible to compute (just stashing a few pieces of
information that already exist into the Index structure), and I would
assume the bitcode writing part is fast. In terms of size, in the
per-module index (produced by the -flto=thin -c step) it is an extra
block containing a single record per function. Right now those records
are small (just containing the ValueID for the associated VST entry, a
bool flag and the instruction count). The records will grow as we find
more metrics that are useful to save and use in guiding ThinLTO
importing.

So it is something that we could consider always including, but I
assumed that until there was another consumer (or until ThinLTO is the
default =) ) that people would not want the extra bitcode overhead.

>
> Thanks!
>
> -eric
>
>
> ================
> Comment at: llvm/trunk/include/llvm/IR/FunctionInfo.h:57
> @@ +56,3 @@
> +  /// Number of instructions (ignoring debug instructions, e.g.) computed
> +  /// during the initial compile step when the function index is first built.
> +  unsigned InstCount;
> ----------------
> Comment nit: "during the initial compile step" seems weird and not wholly accurate.

It is the instruction count computed when we write out the bitcode at
the end of the "-c -flto=thin" compilation step. Is there a better way
to say that? Maybe add "and the bitcode is written" to the end of the
sentence?

>
> Relatedly: would a transform be expected to keep it up to date? Is it recomputed on every write? (Seems reasonable, but costly).

No, it is written once when those intermediate bitcode .o files are
written. It is then stashed in the combined index created during the
initial link step, and then looked at by other modules when making
importing decisions. There isn't any transformation between that "-c
-flto=thin" bitcode write and when it is examined by other modules
(which if they decide to import will import the function as written by
that "-c -flto=thin" compile step).

>
> ================
> Comment at: llvm/trunk/include/llvm/IR/FunctionInfo.h:61-62
> @@ +60,4 @@
> + public:
> +  /// Construct a summary object from summary data expected for all
> +  /// summary records.
> +  FunctionSummary(unsigned NumInsts) : InstCount(NumInsts) {}
> ----------------
> This comment seems a bit confusing. Could you elaborate more?

What I was trying to say (not very clearly) is that the
FunctionSummary constructor should accept as parameters all summary
data that we would always expect to have for a function being
summarized. The other data members of this structure (isLocalFunction
and ModulePath) are initialized separately since they are not used in
certain cases (e.g. per-module vs combined index summaries). Would it
be clearer if I added a second sentence saying something like "Other
data members are initialized separately when they are used." ?


>
>
> Repository:
>   rL LLVM
>
> http://reviews.llvm.org/D13107
>
>
>



-- 
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413