[PATCH] D17212: [ThinLTO] Support for call graph in per-module and combined summary.
Mehdi AMINI via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 8 02:39:04 PST 2016
joker.eph added a comment.
LGTM overall. Still some comments/questions below.
Thanks for the good work!
================
Comment at: include/llvm/IR/FunctionInfo.h:87
@@ +86,3 @@
+ /// GlobalValueSummary constructor.
+ GlobalValueSummary(SummaryKind K, GlobalValue::LinkageTypes Linkage)
+ : Kind(K), Linkage(Linkage) {}
----------------
Can't/shouldn't be 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) {
----------------
why `to be written to...`?
================
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) {
----------------
I think the usual terminology in LLVM is "PGO" (I had to google FDO)
(same below)
================
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,
----------------
`F`?
Also again the "to be written to" seems off to me.
================
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) {}
----------------
Can't you have a single ctor?
`GlobalValueInfo(uint64_t Offset = 0, std::unique_ptr<GlobalValueSummary> Summary = nullptr)`
================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:416
@@ -415,3 +415,3 @@
/// files/sections.
class FunctionIndexBitcodeReader {
DiagnosticHandlerFunction DiagnosticHandler;
----------------
I guess more renaming could have been done here right?
(I really don't mind, but since you cared to rename many places below...)
================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:445
@@ +444,3 @@
+ bool SeenValueSymbolTable = false;
+ uint64_t VSTOffset = 0;
+
----------------
doc
================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5788
@@ +5787,3 @@
+ llvm::make_unique<GlobalVarSummary>(
+ /*HACK*/ GlobalValue::ExternalLinkage);
+ FS->setModulePath(
----------------
Mmmmm....
================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5850
@@ -5656,1 +5849,3 @@
+ llvm::make_unique<GlobalVarSummary>(
+ /*HACK*/ GlobalValue::ExternalLinkage);
FS->setModulePath(ModuleIdMap[ModuleId]);
----------------
Mmmmm (bis)....
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:852
@@ -849,2 +851,3 @@
+ VSTOffsetPlaceholder = WriteValueSymbolTableForwardDecl(Stream);
return VSTOffsetPlaceholder;
}
----------------
I'd write it:
```
if (M->getValueSymbolTable().empty())
return 0;
return WriteValueSymbolTableForwardDecl(Stream);
```
================
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 =
----------------
Why is it no longer valid?
================
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,
----------------
Mind adding a one line comment?
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2486
@@ +2485,3 @@
+ if (!isa<GlobalValue>(U2)) {
+ findRefEdges(U2, VE, RefEdges, Visited);
+ continue;
----------------
Guarantee on the recursion depth?
================
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();
----------------
`if(CS)` could be hoisted out of the loop.
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2572
@@ +2571,3 @@
+ CallGraphEdges[CalleeId] += ScaledCount;
+ RefEdges.erase(CalleeId);
+ }
----------------
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)
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2574
@@ +2573,3 @@
+ }
+ continue;
+ }
----------------
?
================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:2603
@@ -2533,1 +2602,3 @@
+ RefEdges.size();
+
----------------
?
================
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(
----------------
Isn't the opposite that the code is doing?
http://reviews.llvm.org/D17212
More information about the llvm-commits
mailing list