Imported statistics types changes

[vsk]

Mostly looks fine, just two nitpicks.

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp

55

Does this line need to change?

186

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

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

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

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

55–57

exactly.

[vsk]

Lgtm.

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp

55–57

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)
+  if (!CallerNode.Imported) {
+    auto It = NodesMap.find(Caller.getName());
     // Save Caller as a starting node for traversal.
-    NonImportedCallers.push_back(Caller.getName());
+    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 auto & 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;
 }