Imported statistics types changes

[vsk]

Mostly looks fine, just two nitpicks.

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp

55 ↗

(On Diff #66195)

Does this line need to change?

186 ↗

(On Diff #66195)

It would probably be clearer to either change the const auto & to std::unique_ptr<...> & or to use Node.get() here.

[Prazek]

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp

55 ↗

(On Diff #66195)

Yes, because the map stores std::string of functions name. The name from Function can dissapear because the Function can dissapear.
Added extra comment to make it clear.

186 ↗

(On Diff #66195)

The type is different than you think. It is StringMapEntry, so I am pushing pointers to StringMapEntry.
I added type to make it more clear.

[tejohnson]

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp

55 ↗

(On Diff #66195)

The Caller function wouldn't have disappeared by here. The reason it needed to change is that NonImportedCallers was changed to store StringRef instead of a std::string, and we can use the StringRef owned by NodesMap to avoid creating a new copy of the string.

[Prazek]

Addressed comments

[Prazek]

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp

52–59 ↗

(On Diff #66393)

exactly.

[vsk]

Lgtm.

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp

52–59 ↗

(On Diff #66393)

Thanks for explaining

[tejohnson]

LGTM. Thanks for the cleanup!

llvm/trunk/include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h

 // ThinLTO.
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TRANSFORMS_UTILS_IMPORTEDFUNCTIONSINLININGSTATISTICS_H
 #define LLVM_TRANSFORMS_UTILS_IMPORTEDFUNCTIONSINLININGSTATISTICS_H
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringMap.h"
 #include <string>
-#include <unordered_map>
 #include <vector>
 
 namespace llvm {
 class Module;
 class Function;
 /// \brief Calculate and dump ThinLTO specific inliner stats.
 /// The main statistics are:
 /// (1) Number of inlined imported functions,

 
 private:
   /// Creates new Node in NodeMap and sets attributes, or returns existed one.
   InlineGraphNode &createInlineGraphNode(const Function &);
   void calculateRealInlines();
   void dfs(InlineGraphNode &GraphNode);
 
   using NodesMapTy =
-      std::unordered_map<std::string, std::unique_ptr<InlineGraphNode>>;
+      llvm::StringMap<std::unique_ptr<InlineGraphNode>>;
   using SortedNodesTy =
-      std::vector<std::pair<std::string, std::unique_ptr<InlineGraphNode>>>;
-  /// Clears NodesMap and returns vector of elements sorted by
+      std::vector<const NodesMapTy::MapEntryTy*>;
+  /// Returns vector of elements sorted by
   /// (-NumberOfInlines, -NumberOfRealInlines, FunctionName).
   SortedNodesTy getSortedNodes();
 
 private:
   /// This map manage life of all InlineGraphNodes. Unique pointer to
   /// InlineGraphNode used since the node pointers are also saved in the
   /// InlinedCallees vector. If it would store InlineGraphNode instead then the
   /// address of the node would not be invariant.
   NodesMapTy NodesMap;
   /// Non external functions that have some other function inlined inside.
-  std::vector<std::string> NonImportedCallers;
+  std::vector<StringRef> NonImportedCallers;
   int AllFunctions = 0;
   int ImportedFunctions = 0;
   StringRef ModuleName;
 };
 
 } // llvm
 
 #endif // LLVM_TRANSFORMS_UTILS_IMPORTEDFUNCTIONSINLININGSTATISTICS_H

llvm/trunk/lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp

if (!CallerNode.Imported && !CalleeNode.Imported) {
     // don't have to add this to graph. It might be very helpful if you wanna
     // get the inliner statistics in compile step where there are no imported
     // functions. In this case the graph would be empty.
     CalleeNode.NumberOfRealInlines++;
     return;
   }
 
   CallerNode.InlinedCallees.push_back(&CalleeNode);
-  if (!CallerNode.Imported)
-    // Save Caller as a starting node for traversal.
-    NonImportedCallers.push_back(Caller.getName());
+  if (!CallerNode.Imported) {
+    // We could avoid second lookup, but it would make the code ultra ugly.
+    auto It = NodesMap.find(Caller.getName());
+    assert(It != NodesMap.end() && "The node should be already there.");
+    // Save Caller as a starting node for traversal. The string has to be one
+    // from map because Caller can disappear (and function name with it).
+    NonImportedCallers.push_back(It->first());
+  }
 }
 
 void ImportedFunctionsInliningStatistics::setModuleInfo(const Module &M) {
   ModuleName = M.getName();
   for (const auto &F : M.functions()) {
     AllFunctions++;
     ImportedFunctions += int(F.getMetadata("thinlto_src_module") != nullptr);
   }
void ImportedFunctionsInliningStatistics::dump(const bool Verbose) {
 
   Ostream << "------- Dumping inliner stats for [" << ModuleName
           << "] -------\n";
 
   if (Verbose)
     Ostream << "-- List of inlined functions:\n";
 
   for (const auto &Node : SortedNodes) {
-    assert(Node.second->NumberOfInlines >= Node.second->NumberOfRealInlines);
-    if (Node.second->NumberOfInlines == 0)
+    assert(Node->second->NumberOfInlines >= Node->second->NumberOfRealInlines);
+    if (Node->second->NumberOfInlines == 0)
       continue;
 
-    if (Node.second->Imported) {
+    if (Node->second->Imported) {
       InlinedImportedFunctionsCount++;
       InlinedImportedFunctionsToImportingModuleCount +=
-          int(Node.second->NumberOfRealInlines > 0);
+          int(Node->second->NumberOfRealInlines > 0);
     } else {
       InlinedNotImportedFunctionsCount++;
       InlinedNotImportedFunctionsToImportingModuleCount +=
-          int(Node.second->NumberOfRealInlines > 0);
+          int(Node->second->NumberOfRealInlines > 0);
     }
 
     if (Verbose)
       Ostream << "Inlined "
-              << (Node.second->Imported ? "imported " : "not imported ")
-              << "function [" << Node.first << "]"
-              << ": #inlines = " << Node.second->NumberOfInlines
+              << (Node->second->Imported ? "imported " : "not imported ")
+              << "function [" << Node->first() << "]"
+              << ": #inlines = " << Node->second->NumberOfInlines
               << ", #inlines_to_importing_module = "
-              << Node.second->NumberOfRealInlines << "\n";
+              << Node->second->NumberOfRealInlines << "\n";
   }
 
   auto InlinedFunctionsCount =
       InlinedImportedFunctionsCount + InlinedNotImportedFunctionsCount;
   auto NotImportedFuncCount = AllFunctions - ImportedFunctions;
   auto ImportedNotInlinedIntoModule =
       ImportedFunctions - InlinedImportedFunctionsToImportingModuleCount;
 
if (!InlinedFunctionNode->Visited)
       dfs(*InlinedFunctionNode);
   }
 }
 
 ImportedFunctionsInliningStatistics::SortedNodesTy
 ImportedFunctionsInliningStatistics::getSortedNodes() {
   SortedNodesTy SortedNodes;
   SortedNodes.reserve(NodesMap.size());
-
-  for (auto &&Node : NodesMap)
-    SortedNodes.emplace_back(Node.first, std::move(Node.second));
-
-  NodesMap.clear(); // We don't want to leave nullptrs.
+  for (const NodesMapTy::value_type& Node : NodesMap)
+    SortedNodes.push_back(&Node);
 
   std::sort(
       SortedNodes.begin(), SortedNodes.end(),
       [&](const SortedNodesTy::value_type &Lhs,
           const SortedNodesTy::value_type &Rhs) {
-        if (Lhs.second->NumberOfInlines != Rhs.second->NumberOfInlines)
-          return Lhs.second->NumberOfInlines > Rhs.second->NumberOfInlines;
-        if (Lhs.second->NumberOfRealInlines != Rhs.second->NumberOfRealInlines)
-          return Lhs.second->NumberOfRealInlines >
-                 Rhs.second->NumberOfRealInlines;
-        return Lhs.first < Rhs.first;
+        if (Lhs->second->NumberOfInlines != Rhs->second->NumberOfInlines)
+          return Lhs->second->NumberOfInlines > Rhs->second->NumberOfInlines;
+        if (Lhs->second->NumberOfRealInlines != Rhs->second->NumberOfRealInlines)
+          return Lhs->second->NumberOfRealInlines >
+                 Rhs->second->NumberOfRealInlines;
+        return Lhs->first() < Rhs->first();
       });
   return SortedNodes;
 }