[PATCH] D36351: [lld][ELF] Add profile guided section layout

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

This is perhaps a fundamental question, but why do you compute the layout in the linker? It looks like you could create an external tool that computes a desired layout and outputs it as a symbol ordering file.



================
Comment at: lld/ELF/CallGraphSort.cpp:62-95
+  struct Node {
+    Node() = default;
+    Node(const InputSectionBase *IS);
+    std::vector<const InputSectionBase *> Sections;
+    std::vector<EdgeIndex> IncidentEdges;
+    int64_t Size = 0;
+    uint64_t Weight = 0;
----------------
Since CallGraphSort class is local to this file anyway, I don't see a reason to make these classes inside CallGraphSort. You could move this outside of CallGraphSort (but still inside the anonymous namespace) to remove an extra level of namespace.


================
Comment at: lld/ELF/CallGraphSort.cpp:100
+  struct EdgePriorityCmp {
+    std::vector<Edge> &Edges;
+    bool operator()(EdgeIndex A, EdgeIndex B) const {
----------------
ArrayRef is preferred over std::vector<T>&


================
Comment at: lld/ELF/CallGraphSort.cpp:106-107
+
+  typedef std::multiset<EdgeIndex, EdgePriorityCmp> priority;
+  typedef std::unordered_map<EdgeIndex, priority::const_iterator> priority_lookup;
+
----------------
priority -> Priority

priority_lookup -> PriorityLookup


================
Comment at: lld/ELF/CallGraphSort.cpp:115-119
+public:
+  CallGraphSort(DenseMap<std::pair<const Symbol *, const Symbol *>,
+                         uint64_t> &Profile);
+
+  DenseMap<const InputSectionBase *, int> run();
----------------
Usually we write public members before private members.


================
Comment at: lld/ELF/CallGraphSort.cpp:124-125
+CallGraphSort::Node::Node(const InputSectionBase *IS) {
+  Sections.push_back(IS);
+  Size = IS->getSize();
+}
----------------
Sections is always empty, no? Why don't you do

  CallGraphSort::Node::Node(const InputSectionBase *IS) : Sections(IS), Size(IS->getSize()) {}

?


================
Comment at: lld/ELF/CallGraphSort.cpp:162
+  // Create the graph.
+  for (const auto &C : Profile) {
+    const Symbol *FromSym = C.first.first;
----------------
Since `Config->CallGraphProfile` is available here, you don't need to pass it as an argument.


================
Comment at: lld/ELF/CallGraphSort.cpp:217
+  }
+  std::vector<EdgeIndex> &FE = Nodes[CE.From].IncidentEdges;
+
----------------
Use ArrayRef.


================
Comment at: lld/ELF/Driver.cpp:581
+// 18446744073709551615 e d
+//
+template <typename ELFT>
----------------
Remove this trailing empty comment line.


================
Comment at: lld/ELF/Driver.cpp:1059
 
+  if (!Config->CallGraphProfileFile.empty())
+    if (Optional<MemoryBufferRef> Buffer =
----------------
Directly use `getLastArg(OPT_call_graph_profile_file)` and remove `Config->CallGraphProfileFile`.


================
Comment at: lld/ELF/Options.td:54-58
+def call_graph_profile_file: S<"call-graph-profile-file">,
+  HelpText<"Layout sections to optimize the given callgraph">;
+
+def call_graph_profile_sort: F<"call-graph-profile-sort">,
+  HelpText<"Sort sections by call graph profile information">;
----------------
I think these two options should be merged into one option which takes a filename as an argument and enables the feature.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:126-142
+  /*MCSectionELF *Sec =
+      getContext().getELFSection(".note.llvm.callgraph", ELF::SHT_NOTE, 0);
+  Streamer.SwitchSection(Sec);
+  SmallString<256> Out;
+  for (const auto &Edge : CFGProfile->operands()) {
+    raw_svector_ostream O(Out);
+    MDNode *E = cast<MDNode>(Edge);
----------------
Debug code?


================
Comment at: llvm/lib/Transforms/Instrumentation/CFGProfile.cpp:96-99
+  INITIALIZE_PASS_DEPENDENCY(BlockFrequencyInfoWrapperPass)
+  INITIALIZE_PASS_DEPENDENCY(BranchProbabilityInfoWrapperPass)
+  INITIALIZE_PASS_END(CFGProfilePass, "cfg-profile",
+    "Generate profile information from the CFG.", false, false)
----------------
Indentation


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3422
+void GNUStyle<ELFT>::printCGProfile(const ELFFile<ELFT> *Obj) {
+  OS<< "GNUStyle::printCGProfile not implemented\n";
+}
----------------
Please use clang-format.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4032
+    return;
+  auto CGProfile = unwrapOrError(Obj->template getSectionContentsAsArray<Elf_CGProfile>(this->dumper()->getDotCGProfileSec()));
+  for (const Elf_CGProfile &CGPE : CGProfile) {
----------------
80 cols



https://reviews.llvm.org/D36351