[PATCH] D115847: [mlgo][inline] Improve global state tracking

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

> This patch avoids relying on analysis invalidation because it turned out to be too aggressive in some cases.

Can you explain this a bit more?

We shouldn't need to completely recalculate everything when adding a new function. Any new functions have to be children of the current SCC, so we should be able to find them by iterating through the outgoing edges from the current SCC and seeing if we encounter any nodes we haven't seen before.



================
Comment at: llvm/include/llvm/Analysis/InlineAdvisor.h:12
 
+#include "llvm/Analysis/CallGraph.h"
 #include "llvm/Analysis/InlineCost.h"
----------------
do you need this? this is the legacy PM one


================
Comment at: llvm/lib/Analysis/MLInlineAdvisor.cpp:140
+void MLInlineAdvisor::onNodeChange(const LazyCallGraph::Node *Node) {
+  if (ShuttingDown)
     return;
----------------
if `ShuttingDown` is ever true, that we means the object has already been destructed and calling any method on it is bad.
In general having things done at a distance in the destructor of an analysis seems iffy. Keeping everything in the pass's run method should be doable here, we just need to keep track of the nodes in the most recently visited SCC.


================
Comment at: llvm/lib/Analysis/MLInlineAdvisor.cpp:443
+#endif // defined(LLVM_HAVE_TF_AOT) || defined(LLVM_HAVE_TF_API)
\ No newline at end of file

----------------
newline?


================
Comment at: llvm/test/Transforms/Inline/ML/state-tracking-coro.ll:1
+; Based on llvm/test/Transforms/Coroutines/coro-split-02.ll
+; Corosplit will keep f1 and add 3 more functions.
----------------
I'm not sure I'd want to depend on exact coroutine semantics for these tests especially since it seems like they keep changing
but if we don't do that then I don't think there's any other way to test new functions being added to the call graph
maybe it's ok if these tests aren't run normally and you're fine with keeping these up to date if coroutine semantics/pipelines change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115847