[PATCH] D12781: PGO IR-level instrumentation infrastructure

[Sean Silva via llvm-commits]

silvas added a comment.

The use of inheritance is still excessive. For example, PGOUseFunc and PGOGenFunc don't seem to need inheritance. Their "subclass state" would be better as local variables of a free function. `FuncPGOInstrumentation` does not need to be a base class. It can simply be a `FuncInfo` class that manages/caches information about a single function. Both uses of `FuncPGOInstrumentation` seem to call `performOnFunc` before they do anything else, so this appears to be establishing an invariant for the class, so this operation should be performed in the constructor (this is much more natural as `FuncInfo`; the complex construction was confusing when it was a base class).


================
Comment at: lib/Transforms/Instrumentation/CFGMST.h:39
@@ +38,3 @@
+
+  struct CompareEdgeWeight {
+    bool operator()(const std::unique_ptr<Edge> &lhs,
----------------
Use a lambda.

================
Comment at: lib/Transforms/Instrumentation/CFGMST.h:110
@@ +109,3 @@
+          BasicBlock *TargetBB = TI->getSuccessor(i);
+          if ((Critical = isCriticalEdge(TI, i)))
+            Weight = BPI->getEdgeProbability(&*BB, TargetBB)
----------------
Move the declaration for `bool Critical = false;` down one line and simplify to `bool Critical = isCriticalEdge(TI, i);`

================
Comment at: lib/Transforms/Instrumentation/CFGMST.h:138
@@ +137,3 @@
+    // when destination BB is a landing pad.
+    // Clang instrumentation in theory needs the same support. But it just
+    // ignores this issue and instrumenting on the source BB. The coverage
----------------
No need to mention clang here. Please file a bug report for this if there isn't one open already.

================
Comment at: lib/Transforms/Instrumentation/CFGMST.h:161
@@ +160,3 @@
+  // Dump the Debug information about the instrumentation.
+  void dumpEdges(const StringRef Message = StringRef()) const {
+    if (!Message.empty())
----------------
Factor the DEBUG macro usage into a single point of truth here. E.g. factor out a function `printEdges(raw_ostream &OS, const StringRef Message)`. Then change this function to be simply `DEBUG(dumpEdges(dbgs(), Message))`. (this is similar to the pattern used by Value, for example).

This will also give clang-format an easier time.

================
Comment at: lib/Transforms/Instrumentation/CFGMST.h:186
@@ +185,3 @@
+    uint32_t Index = BBInfos.size();
+    auto IterBool = BBInfos.insert(std::make_pair(Src, nullptr));
+    if (IterBool.second) {
----------------
Use std::tie

================
Comment at: lib/Transforms/Instrumentation/CFGMST.h:206
@@ +205,3 @@
+public:
+  CFGMST(Function &Func, BranchProbabilityInfo *_BPI = nullptr,
+         BlockFrequencyInfo *_BFI = nullptr)
----------------
Avoid using reserved names.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:151
@@ +150,3 @@
+  const BasicBlock *DestBB;
+  unsigned Weight;
+  bool InMST;
----------------
Other places seem to use uint64_t for weight. Why `unsigned` here?

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:177
@@ +176,3 @@
+  // Return the information string of this object.
+  virtual const std::string infoString() const {
+    return "Index=" + std::to_string(Index);
----------------
Does this actually need to be virtual? Same for all the other virtual methods in this file.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:217
@@ +216,3 @@
+                          std::to_string(FunctionHash);
+    MST.dumpEdges(Message);
+
----------------
"dump" functions are for debugging and should be conditionalized. For example, I suggest refactoring to allow a call

```
DEBUG(
std::string Message = "Dump Function " + FuncName +
                          " after CFGMST: \t Hash: " +
                          std::to_string(FunctionHash);
MST.dumpEdges(dbgs(), Message);
)
```

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:439
@@ +438,3 @@
+                            std::to_string(FunctionHash);
+      MST.dumpEdges(Message);
+    }
----------------
Avoid having debug code be non-conditional.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:449
@@ +448,3 @@
+  for (unsigned i = 0, j = 0, E = MST.AllEdges.size(); i < E; i++) {
+    const std::unique_ptr<PGOUseEdge> &Ei = MST.AllEdges[i];
+    BasicBlock *InstrBB = getInstrBB(Ei.get());
----------------
Avoid `const std::unique_ptr<PGOUseEdge> &`.

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:654
@@ +653,3 @@
+    PGOGenFunc Func(F, &M, BPI, BFI);
+    Func.instrumentOnFunc();
+  }
----------------
Use a free function `instrumentOneFunc`

================
Comment at: lib/Transforms/Instrumentation/PGOInstrumentation.cpp:685
@@ +684,3 @@
+    PGOUseFunc Func(F, &M, BPI, BFI);
+    Func.setPGOCountOnFunc(PGOReader);
+  }
----------------
Use a free function `setPGOCountOnFunc`


http://reviews.llvm.org/D12781