[PATCH] D17212: [ThinLTO] Support for call graph in per-module and combined summary.
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 8 11:20:19 PST 2016
tejohnson added a comment.
In http://reviews.llvm.org/D17212#369588, @joker.eph wrote:
> Another question: you reported measurements in the description, is it up to date with the new patch?
Not yet, need to update that. The size of the thinlto.bc combined index went up quite a bit percentage wise with the ref graph, especially once I corrected it to recursively look for references, and added in the global variable init refs. Here are the new stats, only have non-PGO so far, will collect PGO as well and update the summary (once I make a fix described below to add linkage types to variables):
For 483.xalancbmk (non-PGO):
Total .o (bitcode) size: +1.24%
Combined .thinlto.bc size: +120%
Aggregate .o + .thinlto.bc size: +3.16%
I gave the last number because the size of the combined index .thinlto.bc file is on the same order of magnitude as the largest individual .o bitcode files, so the increase should be taken in that context.
================
Comment at: include/llvm/IR/FunctionInfo.h:87
@@ +86,3 @@
+ /// GlobalValueSummary constructor.
+ GlobalValueSummary(SummaryKind K, GlobalValue::LinkageTypes Linkage)
+ : Kind(K), Linkage(Linkage) {}
----------------
joker.eph wrote:
> Can't/shouldn't be protected?
Good point, will make protected.
================
Comment at: include/llvm/IR/FunctionInfo.h:108
@@ +107,3 @@
+ /// Record references to be written to the summary section for this
+ /// global value.
+ void addRefEdges(DenseSet<unsigned> &RefEdges) {
----------------
joker.eph wrote:
> why `to be written to...`?
Good catch, this was leftover from the prior patch when it was part of a bitcode writer helper, didn't updated it up after moving here. Will fix.
================
Comment at: include/llvm/IR/FunctionInfo.h:149
@@ +148,3 @@
+ /// by \p CalleeGUID, with \p CalleeInfo including the cumulative profile
+ /// count (across all calls from this function) or 0 if no FDO.
+ void addCallGraphEdge(uint64_t CalleeGUID, CalleeInfo Info) {
----------------
joker.eph wrote:
> I think the usual terminology in LLVM is "PGO" (I had to google FDO)
> (same below)
Will fix...bummer that every compiler I have worked on seems to use a different term! PGO/FDO/PBO...bah.
================
Comment at: include/llvm/IR/FunctionInfo.h:154
@@ +153,3 @@
+
+ /// Record call graph edges to be written to the summary section for \p F.
+ /// The edges are provided as a map from callee ID to \p CalleeInfo,
----------------
joker.eph wrote:
> `F`?
>
> Also again the "to be written to" seems off to me.
Same issue as above, stale comment, will update.
================
Comment at: include/llvm/IR/FunctionInfo.h:216
@@ -126,1 +215,3 @@
+ GlobalValueInfo(uint64_t Offset, std::unique_ptr<GlobalValueSummary> Summary)
+ : Summary(std::move(Summary)), BitcodeIndex(Offset) {}
----------------
joker.eph wrote:
> Can't you have a single ctor?
>
> `GlobalValueInfo(uint64_t Offset = 0, std::unique_ptr<GlobalValueSummary> Summary = nullptr)`
>
Yes, will fix.
================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:416
@@ -415,3 +415,3 @@
/// files/sections.
class FunctionIndexBitcodeReader {
DiagnosticHandlerFunction DiagnosticHandler;
----------------
joker.eph wrote:
> I guess more renaming could have been done here right?
> (I really don't mind, but since you cared to rename many places below...)
Yep, this was part of the TODO list in this patch update. Working on that now. Specifically, I'm changing FunctionIndex* and FunctionInfoIndex* to ModuleSummaryIndex*. I left this for a follow-on update because already the renaming was pretty extensive, and this particular change bleeds into some of the interfaces and variable/field names used in other parts of the compiler (clang etc).
================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:445
@@ +444,3 @@
+ bool SeenValueSymbolTable = false;
+ uint64_t VSTOffset = 0;
+
----------------
joker.eph wrote:
> doc
Will do.
================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5788
@@ +5787,3 @@
+ llvm::make_unique<GlobalVarSummary>(
+ /*HACK*/ GlobalValue::ExternalLinkage);
+ FS->setModulePath(
----------------
joker.eph wrote:
> Mmmmm....
Oops. In my haste to get the renaming done and send this out, forgot that I planned to add a linkage type to the global var init records. Will add now.
================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5850
@@ -5656,1 +5849,3 @@
+ llvm::make_unique<GlobalVarSummary>(
+ /*HACK*/ GlobalValue::ExternalLinkage);
FS->setModulePath(ModuleIdMap[ModuleId]);
----------------
joker.eph wrote:
> Mmmmm (bis)....
Ditto.
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:852
@@ -849,2 +851,3 @@
+ VSTOffsetPlaceholder = WriteValueSymbolTableForwardDecl(Stream);
return VSTOffsetPlaceholder;
}
----------------
joker.eph wrote:
> I'd write it:
> ```
> if (M->getValueSymbolTable().empty())
> return 0;
> return WriteValueSymbolTableForwardDecl(Stream);
> ```
Good idea, will fix.
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2346
@@ -2345,3 +2348,2 @@
// actual bitcode written to the stream).
- assert(FunctionIndex->count(F) == 1);
uint64_t BitcodeIndex =
----------------
joker.eph wrote:
> Why is it no longer valid?
Good catch. I think this got removed when I was using a different data structure here for awhile, and I missed it when restoring the old approach. Will add back.
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2473
@@ -2442,15 +2472,3 @@
-/// \brief Save information for the given function into the function index.
-///
-/// At a minimum this saves the bitcode index of the function record that
-/// was just written. However, if we are emitting function summary information,
-/// for example for ThinLTO, then a \a FunctionSummary object is created
-/// to hold the provided summary information.
-static void SaveFunctionInfo(
- const Function &F,
- DenseMap<const Function *, std::unique_ptr<FunctionInfo>> &FunctionIndex,
- unsigned NumInsts, uint64_t BitcodeIndex, bool EmitFunctionSummary) {
- std::unique_ptr<FunctionSummary> FuncSummary;
- if (EmitFunctionSummary) {
- FuncSummary = llvm::make_unique<FunctionSummary>(NumInsts);
- FuncSummary->setFunctionLinkage(F.getLinkage());
+static void findRefEdges(const User *U, const ValueEnumerator &VE,
+ DenseSet<unsigned> &RefEdges,
----------------
joker.eph wrote:
> Mind adding a one line comment?
Will do.
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2486
@@ +2485,3 @@
+ if (!isa<GlobalValue>(U2)) {
+ findRefEdges(U2, VE, RefEdges, Visited);
+ continue;
----------------
joker.eph wrote:
> Guarantee on the recursion depth?
Beyond the Visited set check earlier? What would you like to see?
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2560
@@ +2559,3 @@
+ for (const auto &OI : I->operands()) {
+ if (CS && CS.isCallee(&OI)) {
+ auto CalledFunction = CS.getCalledFunction();
----------------
joker.eph wrote:
> `if(CS)` could be hoisted out of the loop.
Yes, missed this when I cleaned things up from when I was initially looking for non-call references inside this loop.
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2572
@@ +2571,3 @@
+ CallGraphEdges[CalleeId] += ScaledCount;
+ RefEdges.erase(CalleeId);
+ }
----------------
joker.eph wrote:
> You depend on the order of the calls.
> If an instruction first add a call to a function, but later on will reference it, you won't remove it from RefEdges.
>
> This may be intended and you want to have an accurate count of ref + calls and here you're trying to filter calls out of refedges. However a call instruction could legitimately refer the function (a call passing a function pointer as an argument for instance)
>
>
I don't think the order of calls matters? The reference list is populated once outside of this loop.
On your second point, that is true that this will cause a function both called and referenced in another way to only be recorded as called by this function. Is it important to list both types of references? I was thinking that it was important for importing needs to distinguish the functions being called from the other non-call references, but that essentially the combination of the two are the full reference set of the function. If I should put it in both places, I will need to figure out how to distinguish the two types of function references in findRefEdges.
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2574
@@ +2573,3 @@
+ }
+ continue;
+ }
----------------
joker.eph wrote:
> ?
More cruft left from when I was looking for non-call refs in this loop, will remove
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2603
@@ -2533,1 +2602,3 @@
+ RefEdges.size();
+
----------------
joker.eph wrote:
> ?
Incomplete cleanup of debugging output, will remove.
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2977
@@ +2976,3 @@
+ dyn_cast<FunctionSummary>(FunctionIndex[F]->summary());
+ // Add the aliasee to the reference list of alias
+ FS->addRefEdge(
----------------
joker.eph wrote:
> Isn't the opposite that the code is doing?
Yes, good catch. I think what I initially wanted was to record them this way, but the issue is that the alias doesn't have a separate summary, which is why we are grabbing the aliasee summary here. Will update the comment.
http://reviews.llvm.org/D17212
More information about the llvm-commits
mailing list