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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks accepted this revision.
aeubanks added a comment.
This revision is now accepted and ready to land.

In D115847#3228603 <https://reviews.llvm.org/D115847#3228603>, @mtrofin wrote:

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

Oh I thought this was in reference to `FunctionPropertiesAnalysis` being invalidated too much.

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

An SCC pass should not be adding functions that don't relate to some function in the current SCC. That wouldn't fit the CGSCC pass model and would break LCG. So I think looking around all the nodes in the current SCC to discover new functions is fine.

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

later changes sgtm.



================
Comment at: llvm/lib/Analysis/MLInlineAdvisor.cpp:140
+void MLInlineAdvisor::onNodeChange(const LazyCallGraph::Node *Node) {
+  if (ShuttingDown)
     return;
----------------
mtrofin wrote:
> 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.
ah I missed that


================
Comment at: llvm/test/Transforms/Inline/ML/state-tracking-scc-splits.ll:4
+; RUN:   -S -enable-ml-inliner=development -keep-inline-advisor-for-printing < %s 2>&1 | FileCheck %s
+; REQUIRES: have_tf_api
+;
----------------
not related to this patch but you could add this as a `lit.local.cfg` in the `ML` directory so you don't have to put it on every test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115847