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

[Michael Spencer via llvm-commits]

On Thu, Dec 14, 2017 at 4:58 PM, Zachary Turner <zturner at google.com> wrote:

> Can we use llvm hash tables and sets instead of stl ones?


This can probably use DenseMap, but the set needs the iterator stability
guarantees the stdlib provides.

- Michael Spencer


>
> 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/20171214/18cfade4/attachment-0001.html>