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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:82
+  for (const auto &BB : F)
+    if (BB.hasNPredecessorsOrMore(1) || BB.isEntryBlock())
+      FPI.reIncludeBB(BB, LI);
----------------
kazu wrote:
> 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?
yes - it's an in-flight fix-up. Post-inlining, esp. in the case of `invoke` inlining, former landing pad BBs may become dead. They are still visible if one indiscriminately goes over the list of BBs of the function. But they are dead, so might as well not account them.


================
Comment at: llvm/lib/Analysis/FunctionPropertiesAnalysis.cpp:170
+  FPI.updateAggregateStats(Caller, LI);
+  assert(FPI == FunctionPropertiesInfo::getFunctionPropertiesInfo(Caller, LI));
+}
----------------
kazu wrote:
> 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.
When this check is enabled, the source of the problem this patch fixes would be re-surfaced. We only hit that in a very few cases, so while we want it fixed, it's normally relatively light (like you said). Also, we already have builds with assertions enabled (but not `EXPENSIVE_CHECKS`), so these 2 reasons together make `assert`s  more attractive (readily check-able with no (much) impact)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125841