[llvm] [ThinLTO] Allow importing based on a workload definition (PR #74545)

[Teresa Johnson via llvm-commits]

================
@@ -451,76 +451,79 @@ class WorkloadImportsManager : public ModuleImportsManager {
     auto &ValueInfos = SetIter->second;
     SmallVector<EdgeInfo, 128> GlobWorklist;
     for (auto &VI : llvm::make_early_inc_range(ValueInfos)) {
+      auto It = DefinedGVSummaries.find(VI.getGUID());
+      if (It != DefinedGVSummaries.end() &&
+          IsPrevailing(VI.getGUID(), It->second)) {
+        LLVM_DEBUG(
+            dbgs() << "[Workload] " << VI.name()
+                   << " has the prevailing variant already in the module "
+                   << ModName << ". No need to import\n");
+        continue;
+      }
       auto Candidates =
           qualifyCalleeCandidates(Index, VI.getSummaryList(), ModName);
 
       const GlobalValueSummary *GVS = nullptr;
-      FunctionImporter::ImportFailureReason LastReason =
-          FunctionImporter::ImportFailureReason::None;
-      for (const auto &Candidate : Candidates) {
-        /// We will prefer importing the prevailing candidate, if not, we'll
-        /// still pick the first available candidate. The reason we want to make
-        /// sure we do import the prevailing candidate is because the goal of
-        /// workload-awareness is to enable optimizations specializing the call
-        /// graph of that workload. Suppose a function is already defined in the
-        /// module, but it's not the prevailing variant. Suppose also we do not
-        /// inline it, but we could specialize it to the workload in other ways.
-        /// However, the linker would drop it in the favor of the prevailing
-        /// copy. Instead, by importing the prevailing variant (assuming also
-        /// the use of `-avail-extern-to-local`), we keep the specialization. We
-        /// could alteranatively make the non-prevailing variant local, but the
-        /// prevailing one is also the one for which we would have previously
-        /// collected profiles, making it preferrable.
-        LastReason = Candidate.first;
-        if (Candidate.first == FunctionImporter::ImportFailureReason::None) {
-          const bool Prevailing = IsPrevailing(VI.getGUID(), Candidate.second);
-          if (Prevailing || !GVS) {
-            if (!GVS && !Prevailing)
-              LLVM_DEBUG(dbgs()
-                         << "[Workload] Considering " << VI.name() << " from "
-                         << Candidate.second->modulePath() << " with linkage "
-                         << Candidate.second->linkage()
-                         << " although it's not prevailing, but it's the "
-                            "first available candidate.\n");
-            GVS = Candidate.second;
-            if (Prevailing) {
-              LLVM_DEBUG(dbgs()
-                         << "[Workload] Considering " << VI.name() << " from "
-                         << GVS->modulePath() << " with linkage "
-                         << GVS->linkage() << " because it's prevailing.\n");
-              break;
-            }
-          } else {
-            LLVM_DEBUG(dbgs() << "[Workload] Skipping " << VI.name() << " from "
-                              << Candidate.second->modulePath()
-                              << " with linkage " << Candidate.second->linkage()
-                              << " because it's not prevailing\n");
-          }
-        }
-      }
-      if (!GVS) {
+      auto PotentialCandidates = llvm::map_range(
+          llvm::make_filter_range(
----------------
teresajohnson wrote:

I'm a little concerned about the efficiency of having the two make_filter_range invocations, in a case where we may have thousands of copies of a linkonce_odr, for example. Why not just have a single loop and exit as soon as you find the prevailing copy, then check outside the loop to make sure it is importable? You can also keep track of the first valid candidate too in case there isn't a prevailing copy (e.g. a local).

Oh, also, looking at qualifyCalleeCandidates, we will not import anything with interposable linkage (since it isn't inlineable). I think you may want to import in that case given your use case though (failure reason is FunctionImporter::ImportFailureReason::InterposableLinkage)?

https://github.com/llvm/llvm-project/pull/74545