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

[Zachary Turner via llvm-commits]

Can we use llvm hash tables and sets instead of stl ones?
On Thu, Dec 14, 2017 at 4:47 PM Rui Ueyama via Phabricator <
reviews at reviews.llvm.org> wrote:

> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171215/0fc2e029/attachment.html>