Can we use llvm hash tables and sets instead of stl ones?<br><div class="gmail_quote"><div dir="ltr">On Thu, Dec 14, 2017 at 4:47 PM Rui Ueyama via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">ruiu added a comment.<br>
<br>
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.<br>
<br>
<br>
<br>
================<br>
Comment at: lld/ELF/CallGraphSort.cpp:62-95<br>
+  struct Node {<br>
+    Node() = default;<br>
+    Node(const InputSectionBase *IS);<br>
+    std::vector<const InputSectionBase *> Sections;<br>
+    std::vector<EdgeIndex> IncidentEdges;<br>
+    int64_t Size = 0;<br>
+    uint64_t Weight = 0;<br>
----------------<br>
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.<br>
<br>
<br>
================<br>
Comment at: lld/ELF/CallGraphSort.cpp:100<br>
+  struct EdgePriorityCmp {<br>
+    std::vector<Edge> &Edges;<br>
+    bool operator()(EdgeIndex A, EdgeIndex B) const {<br>
----------------<br>
ArrayRef is preferred over std::vector<T>&<br>
<br>
<br>
================<br>
Comment at: lld/ELF/CallGraphSort.cpp:106-107<br>
+<br>
+  typedef std::multiset<EdgeIndex, EdgePriorityCmp> priority;<br>
+  typedef std::unordered_map<EdgeIndex, priority::const_iterator> priority_lookup;<br>
+<br>
----------------<br>
priority -> Priority<br>
<br>
priority_lookup -> PriorityLookup<br>
<br>
<br>
================<br>
Comment at: lld/ELF/CallGraphSort.cpp:115-119<br>
+public:<br>
+  CallGraphSort(DenseMap<std::pair<const Symbol *, const Symbol *>,<br>
+                         uint64_t> &Profile);<br>
+<br>
+  DenseMap<const InputSectionBase *, int> run();<br>
----------------<br>
Usually we write public members before private members.<br>
<br>
<br>
================<br>
Comment at: lld/ELF/CallGraphSort.cpp:124-125<br>
+CallGraphSort::Node::Node(const InputSectionBase *IS) {<br>
+  Sections.push_back(IS);<br>
+  Size = IS->getSize();<br>
+}<br>
----------------<br>
Sections is always empty, no? Why don't you do<br>
<br>
  CallGraphSort::Node::Node(const InputSectionBase *IS) : Sections(IS), Size(IS->getSize()) {}<br>
<br>
?<br>
<br>
<br>
================<br>
Comment at: lld/ELF/CallGraphSort.cpp:162<br>
+  // Create the graph.<br>
+  for (const auto &C : Profile) {<br>
+    const Symbol *FromSym = C.first.first;<br>
----------------<br>
Since `Config->CallGraphProfile` is available here, you don't need to pass it as an argument.<br>
<br>
<br>
================<br>
Comment at: lld/ELF/CallGraphSort.cpp:217<br>
+  }<br>
+  std::vector<EdgeIndex> &FE = Nodes[CE.From].IncidentEdges;<br>
+<br>
----------------<br>
Use ArrayRef.<br>
<br>
<br>
================<br>
Comment at: lld/ELF/Driver.cpp:581<br>
+// 18446744073709551615 e d<br>
+//<br>
+template <typename ELFT><br>
----------------<br>
Remove this trailing empty comment line.<br>
<br>
<br>
================<br>
Comment at: lld/ELF/Driver.cpp:1059<br>
<br>
+  if (!Config->CallGraphProfileFile.empty())<br>
+    if (Optional<MemoryBufferRef> Buffer =<br>
----------------<br>
Directly use `getLastArg(OPT_call_graph_profile_file)` and remove `Config->CallGraphProfileFile`.<br>
<br>
<br>
================<br>
Comment at: lld/ELF/Options.td:54-58<br>
+def call_graph_profile_file: S<"call-graph-profile-file">,<br>
+  HelpText<"Layout sections to optimize the given callgraph">;<br>
+<br>
+def call_graph_profile_sort: F<"call-graph-profile-sort">,<br>
+  HelpText<"Sort sections by call graph profile information">;<br>
----------------<br>
I think these two options should be merged into one option which takes a filename as an argument and enables the feature.<br>
<br>
<br>
================<br>
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:126-142<br>
+  /*MCSectionELF *Sec =<br>
+      getContext().getELFSection(".note.llvm.callgraph", ELF::SHT_NOTE, 0);<br>
+  Streamer.SwitchSection(Sec);<br>
+  SmallString<256> Out;<br>
+  for (const auto &Edge : CFGProfile->operands()) {<br>
+    raw_svector_ostream O(Out);<br>
+    MDNode *E = cast<MDNode>(Edge);<br>
----------------<br>
Debug code?<br>
<br>
<br>
================<br>
Comment at: llvm/lib/Transforms/Instrumentation/CFGProfile.cpp:96-99<br>
+  INITIALIZE_PASS_DEPENDENCY(BlockFrequencyInfoWrapperPass)<br>
+  INITIALIZE_PASS_DEPENDENCY(BranchProbabilityInfoWrapperPass)<br>
+  INITIALIZE_PASS_END(CFGProfilePass, "cfg-profile",<br>
+    "Generate profile information from the CFG.", false, false)<br>
----------------<br>
Indentation<br>
<br>
<br>
================<br>
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:3422<br>
+void GNUStyle<ELFT>::printCGProfile(const ELFFile<ELFT> *Obj) {<br>
+  OS<< "GNUStyle::printCGProfile not implemented\n";<br>
+}<br>
----------------<br>
Please use clang-format.<br>
<br>
<br>
================<br>
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:4032<br>
+    return;<br>
+  auto CGProfile = unwrapOrError(Obj->template getSectionContentsAsArray<Elf_CGProfile>(this->dumper()->getDotCGProfileSec()));<br>
+  for (const Elf_CGProfile &CGPE : CGProfile) {<br>
----------------<br>
80 cols<br>
<br>
<br>
<br>
<a href="https://reviews.llvm.org/D36351" rel="noreferrer" target="_blank">https://reviews.llvm.org/D36351</a><br>
<br>
<br>
<br>
</blockquote></div>