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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked an inline comment as done.
mtrofin added a comment.

I think there are 2 questions, is that correct? If so:

In D115847#3228512 <https://reviews.llvm.org/D115847#3228512>, @aeubanks wrote:

>> 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?

This refers to the behavior before. Invalidation of the whole analysis (the MLInlineAdvisor one) was still happening too frequently.

> 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.

I think you're referring here to the behavior in the patch, correct (i.e. not to the quoted sentence in the patch description)?

I would prefer not assuming that SCC passes adding functions relate them to the SCC they are currently processing. I realize that's currently the case, but this assumption made me uncomfortable:  I am not sure why this is something that a pass would necessarily respect in the future.

I think there is a partial update approach that is only dependent on the fact that Nodes are only added for the duration of a module-wide cgscc traversal: we remember a "watermark" of the last node and iterate from that watermark up next time we re-enter the inliner. That's a change that can be done incrementally though, from the current change.



================
Comment at: llvm/lib/Analysis/MLInlineAdvisor.cpp:140
+void MLInlineAdvisor::onNodeChange(const LazyCallGraph::Node *Node) {
+  if (ShuttingDown)
     return;
----------------
aeubanks wrote:
> 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.
See the dtor - this just avoids spending time doing inserts due to the abandon-ing of the NotifyOnChangeFunctionAnalysis in the ~MLInlineAdvisor. The object isn't destructed just yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115847