[MLInliner] Simplify NodeCount bookkeeping

[aeubanks]

Rather than doing delta counting of the total number of functions, just increment it when we see a new function.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

Author: Arthur Eubanks (aeubanks)

Changes

Rather than doing delta counting of the total number of functions, just increment it when we see a new function.

---

Full diff: https://github.com/llvm/llvm-project/pull/96576.diff

1 Files Affected:

• (modified) llvm/lib/Analysis/MLInlineAdvisor.cpp (+2-2)

diff --git a/llvm/lib/Analysis/MLInlineAdvisor.cpp b/llvm/lib/Analysis/MLInlineAdvisor.cpp
index 21946572339b9..180fac47760cb 100644
--- a/llvm/lib/Analysis/MLInlineAdvisor.cpp
+++ b/llvm/lib/Analysis/MLInlineAdvisor.cpp
@@ -206,7 +206,6 @@ void MLInlineAdvisor::onPassEntry(LazyCallGraph::SCC *LastSCC) {
   // care about the nature of the Edge (call or ref). `FunctionLevels`-wise, we
   // record them at the same level as the original node (this is a choice, may
   // need revisiting).
-  NodeCount -= static_cast<int64_t>(NodesInLastSCC.size());
   while (!NodesInLastSCC.empty()) {
     const auto *N = *NodesInLastSCC.begin();
     NodesInLastSCC.erase(N);
@@ -215,14 +214,15 @@ void MLInlineAdvisor::onPassEntry(LazyCallGraph::SCC *LastSCC) {
       assert(!N->getFunction().isDeclaration());
       continue;
     }
-    ++NodeCount;
     EdgeCount += getLocalCalls(N->getFunction());
     const auto NLevel = FunctionLevels.at(N);
     for (const auto &E : *(*N)) {
       const auto *AdjNode = &E.getNode();
       assert(!AdjNode->isDead() && !AdjNode->getFunction().isDeclaration());
       auto I = AllNodes.insert(AdjNode);
+      // We've discovered a new function.
       if (I.second) {
+        ++NodeCount;
         NodesInLastSCC.insert(AdjNode);
         FunctionLevels[AdjNode] = NLevel;
       }

[llvmbot]

@llvm/pr-subscribers-mlgo

Author: Arthur Eubanks (aeubanks)

Changes

Rather than doing delta counting of the total number of functions, just increment it when we see a new function.

---

Full diff: https://github.com/llvm/llvm-project/pull/96576.diff

1 Files Affected:

• (modified) llvm/lib/Analysis/MLInlineAdvisor.cpp (+2-2)

diff --git a/llvm/lib/Analysis/MLInlineAdvisor.cpp b/llvm/lib/Analysis/MLInlineAdvisor.cpp
index 21946572339b9..180fac47760cb 100644
--- a/llvm/lib/Analysis/MLInlineAdvisor.cpp
+++ b/llvm/lib/Analysis/MLInlineAdvisor.cpp
@@ -206,7 +206,6 @@ void MLInlineAdvisor::onPassEntry(LazyCallGraph::SCC *LastSCC) {
   // care about the nature of the Edge (call or ref). `FunctionLevels`-wise, we
   // record them at the same level as the original node (this is a choice, may
   // need revisiting).
-  NodeCount -= static_cast<int64_t>(NodesInLastSCC.size());
   while (!NodesInLastSCC.empty()) {
     const auto *N = *NodesInLastSCC.begin();
     NodesInLastSCC.erase(N);
@@ -215,14 +214,15 @@ void MLInlineAdvisor::onPassEntry(LazyCallGraph::SCC *LastSCC) {
       assert(!N->getFunction().isDeclaration());
       continue;
     }
-    ++NodeCount;
     EdgeCount += getLocalCalls(N->getFunction());
     const auto NLevel = FunctionLevels.at(N);
     for (const auto &E : *(*N)) {
       const auto *AdjNode = &E.getNode();
       assert(!AdjNode->isDead() && !AdjNode->getFunction().isDeclaration());
       auto I = AllNodes.insert(AdjNode);
+      // We've discovered a new function.
       if (I.second) {
+        ++NodeCount;
         NodesInLastSCC.insert(AdjNode);
         FunctionLevels[AdjNode] = NLevel;
       }

[mtrofin]

(this comment is meant for line 213, but github doesn't let me, so..) do you mean to assert (!N->isDead())?

[aeubanks]

not in this PR, this doesn't change dead function handling

[aeubanks]

so thinking about this a bit more, it's harder to keep track of the number of live functions after #94815. you can look at CGSCCUpdateResult::DeadFunctions (which we don't currently pass to the InlineAdvisor, but we probably could).

pre-#94815, with this patch, we can't detect if another CGSCC pass will come and delete a function. currently that doesn't matter since the inliner is the only CGSCC pass that deletes functions, but I believe the current logic does allow the inline advisor to detect if another CGSCC pass deletes a function.

is there any reason we don't just use Module.getFunctions().size() for NodeCount?

[mtrofin]

so thinking about this a bit more, it's harder to keep track of the number of live functions after #94815. you can look at CGSCCUpdateResult::DeadFunctions (which we don't currently pass to the InlineAdvisor, but we probably could).

pre-#94815, with this patch, we can't detect if another CGSCC pass will come and delete a function. currently that doesn't matter since the inliner is the only CGSCC pass that deletes functions, but I believe the current logic does allow the inline advisor to detect if another CGSCC pass deletes a function.

is there any reason we don't just use Module.getFunctions().size() for NodeCount?

Hmmm... good question! Can't think of a reason, and looking at the history, it seems we always did manual accounting. One possibility may be that at a point we needed to delay delete-ing Functions to the very end of inlining over the whole Module, but we're past that now.

We'd just need to remove the # of declared ones, because NodeCount is defined only - would that (the # declared) be a constant that can be determined upfront at advisor construction time?

[aeubanks]

so thinking about this a bit more, it's harder to keep track of the number of live functions after #94815. you can look at CGSCCUpdateResult::DeadFunctions (which we don't currently pass to the InlineAdvisor, but we probably could).
pre-#94815, with this patch, we can't detect if another CGSCC pass will come and delete a function. currently that doesn't matter since the inliner is the only CGSCC pass that deletes functions, but I believe the current logic does allow the inline advisor to detect if another CGSCC pass deletes a function.
is there any reason we don't just use Module.getFunctions().size() for NodeCount?

Hmmm... good question! Can't think of a reason, and looking at the history, it seems we always did manual accounting. One possibility may be that at a point we needed to delay delete-ing Functions to the very end of inlining over the whole Module, but we're past that now.

We'd just need to remove the # of declared ones, because NodeCount is defined only - would that (the # declared) be a constant that can be determined upfront at advisor construction time?

Ah that's annoying, we can't count on # declarations being constant since you can introduce calls to intrinsics that didn't have a declaration beforehand.

I'll submit this for now since it's already an issue that MLInlineAdvisor can't handle dead functions outside of the inliner.