Imported statistics types changes

[vsk]

Mostly looks fine, just two nitpicks.

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp

52–59

Does this line need to change?

189

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

52–59

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.

189

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

52–59

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

exactly.

[vsk]

Lgtm.

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp

52–59

Thanks for explaining

[tejohnson]

LGTM. Thanks for the cleanup!

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

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;
 }