<div dir="ltr"><div class="gmail_extra"><div><div class="gmail_signature">On Thu, Dec 14, 2017 at 4:58 PM, Zachary Turner <span dir="ltr"><<a href="mailto:zturner@google.com" target="_blank">zturner@google.com</a>></span> wrote:<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Can we use llvm hash tables and sets instead of stl ones?</blockquote><div><br></div><div>This can probably use DenseMap, but the set needs the iterator stability guarantees the stdlib provides.</div><div><br></div><div>- Michael Spencer<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5"><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" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);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-<wbr>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-<wbr>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-<wbr>119<br>
+public:<br>
+  CallGraphSort(DenseMap<std::<wbr>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-<wbr>125<br>
+CallGraphSort::Node::Node(<wbr>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(<wbr>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-><wbr>CallGraphProfileFile.empty())<br>
+    if (Optional<MemoryBufferRef> Buffer =<br>
----------------<br>
Directly use `getLastArg(OPT_call_graph_<wbr>profile_file)` and remove `Config->CallGraphProfileFile`<wbr>.<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/<wbr>TargetLoweringObjectFileImpl.<wbr>cpp:126-142<br>
+  /*MCSectionELF *Sec =<br>
+      getContext().getELFSection(".<wbr>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/<wbr>Instrumentation/CFGProfile.<wbr>cpp:96-99<br>
+  INITIALIZE_PASS_DEPENDENCY(<wbr>BlockFrequencyInfoWrapperPass)<br>
+  INITIALIZE_PASS_DEPENDENCY(<wbr>BranchProbabilityInfoWrapperPa<wbr>ss)<br>
+  INITIALIZE_PASS_END(<wbr>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/<wbr>ELFDumper.cpp:3422<br>
+void GNUStyle<ELFT>::<wbr>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/<wbr>ELFDumper.cpp:4032<br>
+    return;<br>
+  auto CGProfile = unwrapOrError(Obj->template getSectionContentsAsArray<Elf_<wbr>CGProfile>(this->dumper()-><wbr>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/<wbr>D36351</a><br>
<br>
<br>
<br>
</blockquote></div>
</div></div></blockquote></div><br></div></div>