[PATCH] D125841: [mlgo] Incrementally update FunctionPropertiesInfo during inlining

[Kazu Hirata via Phabricator via llvm-commits]

kazu added inline comments.


================
Comment at: llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h:118
+  FunctionPropertiesInfo &FPI;
+  const BasicBlock &Entry;
+  const Function &Caller;
----------------
Could we rename this variable to `CallSiteBB` or something?  I got confused on a statement like `&Entry != &*Caller.begin()` below.




================
Comment at: llvm/include/llvm/Analysis/FunctionPropertiesAnalysis.h:121
+
+  DenseSet<const BasicBlock *> Changed;
+  DenseSet<const BasicBlock *> Successors;
----------------
Is this variable dead?  If so, please remove it.


================
Comment at: llvm/include/llvm/Analysis/MLInlineAdvisor.h:74
   }
+  mutable DenseMap<const Function *, FunctionPropertiesInfo> FPICache;
 
----------------
May I suggest an empty line between a function definition and a member variable declaration?


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:15
 #include "llvm/Analysis/FunctionPropertiesAnalysis.h"
+#include "llvm/ADT/DepthFirstIterator.h"
+#include "llvm/ADT/STLExtras.h"
----------------
Are we using this?  If not, please remove it.


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:34
+}
+int64_t getUses(const Function &F) {
+  return ((!F.hasLocalLinkage()) ? 1 : 0) + F.getNumUses();
----------------
May I suggest an empty line here between the two function definitions?


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:64-66
+  TotalInstructionCount +=
+      (Direction * std::distance(BB.instructionsWithoutDebug().begin(),
+                                 BB.instructionsWithoutDebug().end()));
----------------
Could we use `sizeWithoutDebug` instead?

```
TotalInstructionCount += Direction * BB.sizeWithoutDebug();
```


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:82
+  for (const auto &BB : F)
+    if (BB.hasNPredecessorsOrMore(1) || BB.isEntryBlock())
+      FPI.reIncludeBB(BB, LI);
----------------
Could we use `!pred_empty(&BB)`?

That said, the original code does not check whether the predecessor list is empty or not, but your version does.  Does that matter?


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:159
+  Worklist.push_back(&Entry);
+  while (!Worklist.empty()) {
+    const auto *BB = Worklist.front();
----------------
Could we have some comment?

```
// Update the properties of those basic blocks that are likely modified or copied from the callee.
```



================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:166-167
+    if (!Successors.contains(BB))
+      for (auto I = succ_begin(BB), E = succ_end(BB); I != E; ++I)
+        Worklist.push_back(*I);
+  }
----------------
```
for (const BasicBlock *Succ : successors(BB))
  Worklist.push_back(Succ);
```


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:170
+  FPI.updateAggregateStats(Caller, LI);
+  assert(FPI == FunctionPropertiesInfo::getFunctionPropertiesInfo(Caller, LI));
+}
----------------
Should we hide this assert under `EXPENSIVE_CHECKS` or something?  IIUC, `FunctionPropertiesInfo::getFunctionPropertiesInfo` visits all of the caller's basic blocks, so we might trigger a quadratic behavior when the caller has many call sites.

That said, this check may be much lighter than, say, the BFS traversal of the callee in `InlineCost.cpp`, so I am not sure how much we should worry about this.


================
Comment at: llvm/lib/Analysis/MLInlineAdvisor.cpp:387
+      PreInlineCallerFPI(Advisor->getCachedFPI(*Caller)),
+      FPU(Advisor->getCachedFPI(*getCaller()), CB) {}
+
----------------
I am not sure how significant this is, but the constructor for `FPU` may visit a few basic blocks (and possibly some memory allocations).  It would be nice if we could avoid invoking this constructor when `Recommendation` is `false`.


================
Comment at: llvm/lib/Analysis/MLInlineAdvisor.cpp:435
 void MLInlineAdvice::recordUnattemptedInliningImpl() {
+  getAdvisor()->getCachedFPI(*Caller) = PreInlineCallerFPI;
   ORE.emit([&]() {
----------------
This comment is related to my comment on `MLInlineAdvice::MLInlineAdvice` above.  If we are not modifying the IR here at all, the code would look clearer without this assignment statement.

That said, we can remove this line if and only if we avoid calling the constructor `FPU` for `Recommendation == false` cases.

If we turn `FPU` into `llvm::optional`, then we might consider:

```
assert(!FPU.hasValue())
```

to ensure that we don't subtract things and walk away without adding things back.

By the way, we still have to stick to `llvm::optional` because `std::optional` is in C++ 17, which is beyond LLVM Coding Standards allows at the moment. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125841/new/

https://reviews.llvm.org/D125841